Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow a full deployment not called 'prod' for pex deploys #155

Merged
merged 4 commits into from
Oct 13, 2023

Conversation

shalabhc
Copy link
Contributor

@shalabhc shalabhc commented Oct 10, 2023

The default deployment name can be overridden using the deployment: input to the action. Eg

image

Test Plan

Rename a deployment to something other than prod
Point the build_deploy_python_executable action in deploy.yml to this branch and add the deployment: input
Verify that the workflow deployment works
Rename deployment back to prod
Verify that workflow deployment fails
Remove deployment: input
Verify that the workflow deployment works again

Ran all above steps in a test org successfully.

@github-actions
Copy link

github-actions bot commented Oct 10, 2023

Your pull request is automatically being deployed to Dagster Cloud.

Location Status Link Updated
from_gh_action Deploy failed Oct 13, 2023 at 05:58 PM (UTC)

@gibsondan
Copy link
Member

How suspicious is the test failure here?

@johannkm
Copy link

This is different from how you set it for hybrid deploys right? Could we make both use this env var?

Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also different than how you set it for Docker deploys within serverless, agree that consistency would be ideal here:

https://github.com/dagster-io/dagster-cloud-action/blob/main/actions/serverless_prod_deploy/action.yml#L19-L22

We should also set this automatically when a new github action is created automatically from Dagster Cloud and the deployment isn't prod

Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see inline

@shalabhc
Copy link
Contributor Author

ah good points re consistency. I'll make this self consistent with how we do serverless docker deploys using the input field instead.

eventually i want to replace the serverless workflow entirely with the new workflows we use for hybrid (https://github.com/dagster-io/dagster-cloud-hybrid-quickstart/) but haven't gotten around to testing, releasing and documenting that.

@shalabhc
Copy link
Contributor Author

@gibsondan - made this consistent with docker deploys using a new deployment: input. Tested manually successfully:
image
image

The test failures have been happening for a while and we haven't gotten around to identifying or fixing them yet.

@shalabhc shalabhc requested a review from gibsondan October 13, 2023 18:33
Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks - can you fill in the test plan? probably something like 'make a new serverless deployment that isn't prod, update the deploy.yml, it deploys to the correct place?'

deployment_name = None
# INPUT_DEPLOYMENT is to the `deployment:` input value in action.yml
deployment_name = os.getenv("INPUT_DEPLOYMENT", "prod")
print(f"Deploying to a full deployment: {deployment_name}", flush=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Deploying to a deployment" reads a bit oddly

@shalabhc shalabhc merged commit 486de0d into main Oct 13, 2023
4 of 9 checks passed
@shalabhc shalabhc deleted the shalabhc/fix-full-branch-deployment-name branch October 13, 2023 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants