-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
feat: add gh deployment
commands
#8718
base: trunk
Are you sure you want to change the base?
Conversation
Sorry for the delay on reviewing this @sjparkinson, I hope to prioritise it tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sjparkinson sorry for the super super super slow review. Thank yuo so much for having a run at this.
I haven't closely looked at the code implementation choices you've made though generally it looks pretty clear what's going on. Before I spend the time on that, I wanted to run some product questions by you. Keep in mind I am not a domain expert on deployments or how you use them.
Long: heredoc.Docf(` | ||
Create a new deployment. | ||
|
||
This will dispatch a %[1]sdeployment%[1]s event that external services can listen for and act on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question
Should what creating a deployment does be documented here? Seems like maybe the concern of the github docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to keep this minimal. This line was stolen from https://docs.github.com/en/rest/deployments/deployments?apiVersion=2022-11-28#about-deployments. Would "Create a new deployment." be enough?
Thinking back, I included this to try and help clarify that this is related to the GitHub Deployment API, given how "deployment" is such a generic term otherwise.
Short: "List deployments", | ||
Long: heredoc.Docf(` | ||
List deployments. | ||
`, "`"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change Requested
Make this a heredoc.Doc
since right now it produces:
List deployments.
%!(EXTRA string=`)
}, | ||
} | ||
|
||
cmd.Flags().IntVarP(&opts.Limit, "limit", "L", 10, "Maximum number of deployments to fetch") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question
I don't think it needs to be as part of this PR but would you agree that it might make sense to have flags for:
sha
ref
task
As documented https://docs.github.com/en/rest/deployments/deployments?apiVersion=2022-11-28#list-deployments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you totally could. Though none I'd likely use personally (especially task
).
I kept to environment
only as that aligns to what you can filter by in the GitHub UI (in the current and beta versions of it).
}, | ||
} | ||
|
||
cmd.Flags().StringVarP(&opts.Ref, "ref", "r", "", "The `branch`, `tag` or `SHA` to deploy (default [current branch])") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question
I don't think it needs to be as part of this PR but would you agree that it might make sense to have flags for:
task
auto_merge
required_contexts
paylod
description
transient_environment
production_environment
As documented https://docs.github.com/en/rest/deployments/deployments?apiVersion=2022-11-28#create-a-deployment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly? In practice I'm not certain I'd make use of them.
To add context to my thinking on what has been included, my intent with this change is to be able to recreate what is achieved in GitHub Actions when environment
is defined for a job in other CI providers such as CircleCI using the gh
CLI. In Actions only the environment name and URL can be specified1, though likely some other fields are automatically populated?
Footnotes
If the repository only has one deployment, you can delete the | ||
deployment regardless of its status. If the repository has more | ||
than one deployment, you can only delete inactive deployments. | ||
`, "`"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change Requested
Same as list
:
Delete a deployment.
If the repository only has one deployment, you can delete the
deployment regardless of its status. If the repository has more
than one deployment, you can only delete inactive deployments.
%!(EXTRA string=`)
|
||
cmd.Flags().StringVarP(&opts.Ref, "ref", "r", "", "The `branch`, `tag` or `SHA` to deploy (default [current branch])") | ||
cmd.Flags().StringVarP(&opts.Environment, "environment", "e", "", "The `environment` that the deployment is for") | ||
cmd.Flags().StringVarP(&opts.Status, "status", "s", "", "The initial `status` of the deployment") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question
Should this actually be called --initial-status
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on this, either works for me.
|
||
cmd.Flags().StringVarP(&opts.Ref, "ref", "r", "", "The `branch`, `tag` or `SHA` to deploy (default [current branch])") | ||
cmd.Flags().StringVarP(&opts.Environment, "environment", "e", "", "The `environment` that the deployment is for") | ||
cmd.Flags().StringVarP(&opts.Status, "status", "s", "", "The initial `status` of the deployment") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question
I see that this command takes status
but nothing else from the deployment status endpoints: https://docs.github.com/en/rest/deployments/statuses?apiVersion=2022-11-28#create-a-deployment-status
Is this trying to solve for some common use case? I think what I'm generally confused about is that as per the docs there is a distinct separation between Tooling
creating a deployment and 3rd Party
creating a deployment status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what I'm generally confused about is that as per the docs there is a distinct separation between
Tooling
creating a deployment and3rd Party
creating a deployment status.
Me too! Again, my use case and line of thinking is very much to enable what happens within GitHub Actions. Where "Tooling" and "3rd Party" in that diagram are effectively the same (edit: unless your workflow runs on the deployment event...)?
My understanding is the docs, and the API, was designed to support deployment workflows like you have at GitHub. The ChatOps style, request a branch to be deployed to an environment.
Whereas what is in the GitHub Actions integration, and what I work with is a more standard continuous deployment workflow. Where only a continuous delivery provider is involved, no extra tooling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this trying to solve for some common use case?
Yep, in a continuous delivery workflow, for a push to main
, you are always "create deployment -> create in_progress/queued deployment status". This is a convenience flag.
Use: "update [flags] <deployment>", | ||
Short: "Update a deployment", | ||
Long: heredoc.Docf(` | ||
Update a deployment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question
This mostly looks like it's related to deployment statuses
, which also have the ability to be listed and deleted. How would we model that functionality in gh
if it were asked for given that you've decided to include creation of a deployment status inside the deployment
command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's some overlap here with #8718 (comment) and the point on a sub-command structure, e.g. gh deployment status delete 132143234
though my gut feeling at the time was it might be one layer of commands too deep?
If not I suspect it'd be a simpler design to align to the Deployment API which would sort out the questions on how to add support for more status
and environment
endpoints.
"github.com/spf13/cobra" | ||
) | ||
|
||
func NewCmdDeployment(f *cmdutil.Factory) *cobra.Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Food for thought
I wonder if the very next thing we're going to get asked is "can I create an environment" or "can I create a status" and then maybe we're going to end up having to figure out how to model those in the command structure as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair questions, and I was wondering about a sub-command to align to the Deployment API and it's various endpoints.
Though I've sort of figured the various endpoints are a bit clunky, for my use cases, and in replicating them as-is in the CLI it'd be surfacing that complexity to users rather than simplifying it. Similar to how GitHub Actions seems to radically simplify the usage of the Deployments API too.
This thinking comes from reading about the intent of this CLI to be a bit opinionated about workflows and simplify the experience1.
Footnotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On "can I create an environment" specifically, it does seem across the GitHub UI and API that the behaviour is to automatically create an environment if a new one is referenced without any error. So I'd be mindful of how that fits in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I've sort of figured the various endpoints are a bit clunky, for my use cases, and in replicating them as-is in the CLI it'd be surfacing that complexity to users rather than simplifying it.
100%. I think the question is "how complex is it going to be if people want that as first class behaviour". Are we going to tell them to use gh api
? Are we going to end up having some mixing of abstraction layers in the commands? Not sure yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also worth highlighting gh environment
already exists as a command for another use-case if the line of thinking is towards more top-level commands e.g. gh deployment-status create
or the like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjparkinson @williammartin : I'd like to propose pivoting back to the issue in order to discuss some thoughts about what deployment support might look like with an eye to GitHub UI experience. I think this PR is a good start, but there are pieces of the experience missing that I'd prefer capturing with screenshots and aligning with others feedback.
Fixes #5541.
Manage GitHub deployments using
gh deployment
commands.https://docs.github.com/en/rest/deployments/deployments
This is my first serious bit of Go and I made good use of Copilot to get though some of this so feedback very welcome!
Usage
Basic overview of the four commands, further detail is included as examples in each command.