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

feat: add gh deployment commands #8718

Open
wants to merge 11 commits into
base: trunk
Choose a base branch
from

Conversation

sjparkinson
Copy link

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.

# Create a deployment on the current branch
$ gh deployment create --status in_progress

# Update a deployment to success
$ gh deployment update --status success 1234

# List deployments in the current repo
$ gh deployment list

# Delete a deployment
$ gh deployment delete 1234

@sjparkinson sjparkinson requested a review from a team as a code owner February 19, 2024 19:50
@sjparkinson sjparkinson requested review from williammartin and removed request for a team February 19, 2024 19:50
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Feb 19, 2024
@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Feb 19, 2024
@williammartin
Copy link
Member

Sorry for the delay on reviewing this @sjparkinson, I hope to prioritise it tomorrow.

Copy link
Member

@williammartin williammartin left a 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.
Copy link
Member

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?

Copy link
Author

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.
`, "`"),
Copy link
Member

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")
Copy link
Member

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

Copy link
Author

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])")
Copy link
Member

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

Copy link
Author

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

  1. https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idenvironment

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.
`, "`"),
Copy link
Member

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")
Copy link
Member

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?

Copy link
Author

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")
Copy link
Member

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.

Copy link
Author

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 and 3rd 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.

Copy link
Author

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.
Copy link
Member

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?

Copy link
Author

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 {
Copy link
Member

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.

Copy link
Author

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

  1. https://github.com/cli/cli/blob/trunk/docs/gh-vs-hub.md

Copy link
Author

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.

Copy link
Member

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.

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external pull request originating outside of the CLI core team
Projects
No open projects
The GitHub CLI
  
Needs review 🤔
Development

Successfully merging this pull request may close these issues.

Add gh deployment
3 participants