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.

Sign up for GitHub

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

Jump to bottom

gh repo delete #4451

Merged
merged 8 commits into from Oct 19, 2021
Merged

gh repo delete #4451

merged 8 commits into from Oct 19, 2021

Conversation

meiji163
Copy link
Contributor

@meiji163 meiji163 commented Oct 6, 2021

Add gh repo delete command. Fixes #3625

@meiji163 meiji163 self-assigned this Oct 7, 2021
@meiji163 meiji163 added core This issue is not accepting PRs from outside contributors enhancement a request to improve CLI labels Oct 7, 2021
@meiji163
Copy link
Contributor Author

meiji163 commented Oct 7, 2021 •
edited

Usage is based on the extension https://github.com/mislav/gh-delete-repo .

To confirm deletion user types the full repo name, or passes in --confirm flag (for scripting purposes)

@meiji163 meiji163 marked this pull request as ready for review October 12, 2021 18:16
@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Oct 12, 2021
@vilmibm vilmibm self-requested a review October 12, 2021 19:23
@billygriffin billygriffin mentioned this pull request Oct 12, 2021
Closed
samcoe
samcoe reviewed Oct 13, 2021
View reviewed changes
Copy link
Contributor

@samcoe samcoe left a comment

Choose a reason for hiding this comment

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

This is looking great so far! Left some comments/questions for discussion.

pkg/cmd/repo/delete/delete.go Outdated Show resolved Hide resolved
pkg/cmd/repo/delete/delete.go Outdated Show resolved Hide resolved
pkg/cmd/repo/delete/delete.go Outdated Show resolved Hide resolved
pkg/cmd/repo/delete/delete.go Outdated Show resolved Hide resolved
pkg/cmd/repo/delete/delete.go Outdated Show resolved Hide resolved
mislav
mislav suggested changes Oct 13, 2021
View reviewed changes
pkg/cmd/repo/delete/delete.go Outdated Show resolved Hide resolved
pkg/cmd/repo/delete/delete.go Outdated Show resolved Hide resolved
pkg/cmd/repo/delete/delete.go Outdated Show resolved Hide resolved
pkg/cmd/repo/delete/delete.go Outdated Show resolved Hide resolved
vilmibm
vilmibm suggested changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@vilmibm vilmibm left a comment

Choose a reason for hiding this comment

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

This is coming along great! In addition to Sam's observations, I have one more request:

It'd be great to notice when the API call 403s and suggest that the user run gh auth refresh -sdelete_repo to fix it; for all other HTTP error status codes, just printing the error as is done now is fine.

@meiji163
Copy link
Contributor Author

@vilmibm

It'd be great to notice when the API call 403s and suggest that the user run gh auth refresh -sdelete_repo to fix it; for all other HTTP error status codes, just printing the error as is done now is fine.

I originally had this and @mislav suggested that it wasn't necessary. He said he is working on general scope error handling. Should I put it in for now?

@mislav mislav mentioned this pull request Oct 13, 2021
Merged
@meiji163 meiji163 requested a review from a team as a code owner October 15, 2021 04:27
mislav
mislav suggested changes Oct 15, 2021
View reviewed changes
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Looks great! One last bit about error handling

pkg/cmd/repo/delete/http.go Outdated Show resolved Hide resolved
@meiji163 @mislav
scope error handling
f7b4588 
Co-authored-by: Mislav Marohnić <mislav@github.com>
@vilmibm
Copy link
Contributor

vilmibm commented Oct 15, 2021 •
edited

@mislav Your code doesn't handle this case, hence why I specifically asked meiji163 yesterday to handle the 403 in the way he did; I don't see a way around the explicit error handling. Do you disagree that we should suggest the scope refresh in cases like this where the 403 doesn't have a missing scopes header?

sorry for my confusion: I read in the error handling PR that this case wouldn't be handled and then, locally, didn't realize that trunk hadn't been merged into this PR so I didn't see the new behavior. Sorry for all the churn.

@vilmibm
Copy link
Contributor

vilmibm commented Oct 19, 2021

Since @mislav 's final change request was addressed, I'm going to go ahead and merge this.

@vilmibm vilmibm merged commit 5a3b9ce into trunk Oct 19, 2021
The GitHub CLI automation moved this from Needs review 🤔 to Pending Release 🥚 Oct 19, 2021
@vilmibm vilmibm deleted the repo-delete branch October 19, 2021 16:13
@thunder-coding
Copy link

thunder-coding commented Oct 19, 2021 •
edited

Thanks for adding the feature 🥳🥳🎉🎉

@mislav mislav mentioned this pull request Oct 19, 2021
Merged
@github-actions github-actions bot moved this from Pending Release 🥚 to Done 💤 in The GitHub CLI Oct 25, 2021
@pomnt
Copy link

pomnt commented Oct 31, 2021

Add gh repo delete command. Fixes #3625

@jenkins-infra-bot jenkins-infra-bot mentioned this pull request Nov 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samcoe samcoe samcoe left review comments

@vilmibm vilmibm vilmibm approved these changes

@mislav mislav Awaiting requested review from mislav

Assignees

@meiji163 meiji163

Labels
core This issue is not accepting PRs from outside contributors enhancement a request to improve CLI
Projects
No open projects
The GitHub CLI
  
Done 💤
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature Request: gh repo delete
6 participants
@meiji163 @vilmibm @thunder-coding @pomnt @mislav @samcoe