Add base_deployment_name to get branch deployment action#232
Add base_deployment_name to get branch deployment action#232
Conversation
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
Your pull request is automatically being deployed to Dagster Cloud.
|
gibsondan
left a comment
gibsondan
left a comment
There was a problem hiding this comment.
see inline - wondering if we should strive for consistency with the 'dagster-cloud ci init' behavior here?
It is admittedly odd that that does not apply to the 'dagster-cloud branch-deployment' command
| @@ -62,6 +62,7 @@ runs: | |||
| run: > | |||
There was a problem hiding this comment.
i think in other contexts we use this input to be the base deployment to use if its a PR, and the actual deployment name to be used if its a full deployment. I have mixed feelings about that but we should probably be consistent? i.e. can we get away without needing a new input here?
See for example https://github.com/dagster-io/dagster-cloud-action/blob/main/actions/utils/ci-init/action.yml#L11-L14 from the 'new' github action flow
If we do add an input we should document it here
There was a problem hiding this comment.
Updated this action to re-use the same input. The others it's a little tricky to, so I left the new input.
I can't say I have a great understanding of which actions are in use right now - at some point we can deprecate the non ci ones, right?
gibsondan
left a comment
gibsondan
left a comment
There was a problem hiding this comment.
hm ok sorry for the back and forth here - if we can't fully port it over (which is not the end of the world) I think inconsistency between the old dagster-cloud action and the new dagster-cloud action setup is preferable to inconsistency within a single action YAML file (i.e. if I am understanding htis now, you would set it in two different ways in this one file here, one for the PEX flow and one for the docker flow, which seems even more confusing: https://github.com/dagster-io/dagster/blob/bb6628650877f9ef7a780ebc2e6a349c92c03f2c/examples/assets_dbt_python/.github/workflows/examples/branch_deployments.yml#L1-L86)
So i think going back to the old way where we were always explicitly passing in the base_deployment_name everywhere is fine (and arguably how the new CLI flow should work too)
Hoping we can fully move off of these old actions soon, the agent heartbeat fix is an attempt to remove the last blocker there
2 participants
Summary
Adds the option to configure a
base_deployment_namefor the serverless, hybrid deploy actions and the get branch deployment action.Test Plan
Tested against prod with test org.