Skip to content

Add label clone command - #5452

Merged
samcoe merged 7 commits into
cli:trunkfrom
heaths:issue5441
Apr 25, 2022
Merged

Add label clone command#5452
samcoe merged 7 commits into
cli:trunkfrom
heaths:issue5441

Conversation

@heaths

@heaths heaths commented Apr 13, 2022

Copy link
Copy Markdown
Contributor

Resolves #5441

@heaths

heaths commented Apr 13, 2022

Copy link
Copy Markdown
Contributor Author

@samcoe if you take #5451 I'll probably refactor into a single package to reuse many of the same private funcs much like the codespaces command does. Could use list and then create in a loop which will automatically take advantage of the --force switch support I added.

@samcoe

samcoe commented Apr 13, 2022

Copy link
Copy Markdown
Contributor

@heaths I think #5451 is a good addition and it makes sense to extract out common functionality into a shared or common file which we do in a couple different commands 👍

@heaths

heaths commented Apr 13, 2022

Copy link
Copy Markdown
Contributor Author

I could do that as well. The funcs would have to be public, of course, but all that is under internal/ anyway.

@heaths heaths changed the title WIP: Add label clone command Add label clone command Apr 20, 2022
@heaths

heaths commented Apr 20, 2022

Copy link
Copy Markdown
Contributor Author

@samcoe wanted to get your preliminary feedback on this. Once I incorporate any feedback I will add tests and convert this to a real PR.

@samcoe samcoe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@heaths This is coming along nicely. I left a handful of comments but nothing major I don't think. Let me know if you have any questions.

Comment thread api/queries_repo.go
Comment thread internal/ghrepo/repo.go
Comment thread pkg/cmd/label/clone.go
Comment thread pkg/cmd/label/clone.go Outdated
Comment thread pkg/cmd/label/clone.go Outdated
Comment thread pkg/cmd/label/clone.go Outdated
Comment thread pkg/cmd/label/clone.go Outdated
Comment thread pkg/cmd/label/clone.go Outdated
Comment thread pkg/cmd/label/http.go
@heaths heaths mentioned this pull request Apr 20, 2022
3 tasks
@heaths
heaths marked this pull request as ready for review April 21, 2022 20:19
@heaths
heaths requested a review from a team as a code owner April 21, 2022 20:19
@heaths
heaths requested review from samcoe and removed request for a team April 21, 2022 20:19
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Apr 21, 2022
@samcoe samcoe self-assigned this Apr 22, 2022

@samcoe samcoe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@heaths Appreciate the hard work here. This looks great, thanks for the thorough tests. I pushed a small commit to polish up a couple tiny things rather than going through another round of review. I will get this merged before the next release.

@samcoe

samcoe commented Apr 25, 2022

Copy link
Copy Markdown
Contributor

Some thoughts for future improvements that we might want to address after testing out the command:

  1. Parallelizing the creation of labels. Even on a small amount of labels (40) the command can take seconds to run.
  2. Adding in a confirmation prompt, generally we reserve this for destructive actions, but I can see how it would be valuable since it can be super tedious to undo (deleting) numerous labels.

I don't think either needs to be addressed right away just food for thought.

@heaths

heaths commented Apr 25, 2022

Copy link
Copy Markdown
Contributor Author

I will get this merged before the next release.

@samcoe, anything holding it up now? I have another PR that is based on this work to improve the label list command and add the label remove command, but didn't want to make it a full PR until I can rebase onto trunk from where it diverged this with one. IMO, making the label command complete by next release would be ideal.

Parallelizing the creation of labels. Even on a small amount of labels (40) the command can take seconds to run.

I've never actually tested this nor seen it in docs, but is there a simultaneous connection limit per client ID for GitHub APIs? And, if so, is it different for REST vs. GraphQL? This could significantly impact the design. Could either go with a fixed-sized goroutine pool or just span goroutines as needed e.g. per label creation/update. Normally I'd lean toward the former.

Comment thread pkg/cmd/label/clone_test.go Outdated
Comment thread pkg/cmd/label/http.go Outdated
@heaths

heaths commented Apr 25, 2022

Copy link
Copy Markdown
Contributor Author

Rebased on trunk to resolve merge conflicts.

@samcoe
samcoe enabled auto-merge (squash) April 25, 2022 18:13
@samcoe
samcoe merged commit 19a852a into cli:trunk Apr 25, 2022
@heaths
heaths deleted the issue5441 branch April 25, 2022 18:17
@heaths

heaths commented Apr 25, 2022

Copy link
Copy Markdown
Contributor Author

@samcoe here the other one now that I've rebased it onto trunk: #5503

@heaths heaths mentioned this pull request Apr 26, 2022
@adriens

adriens commented Apr 27, 2022

Copy link
Copy Markdown

Documented in https://cli.github.com/manual/gh_label_clone
Very useful ❣️ Thanks a lot.

It helps on community/community#15300

@adriens

adriens commented Apr 27, 2022

Copy link
Copy Markdown

🎁 Thanks a lot for the feature 🙏
Here is my feedback : https://dev.to/adriens/github-cli-v290-is-out-clone-labels-feature-5452-is-here-3odk

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

None yet

Development

Successfully merging this pull request may close these issues.

Add label clone command

4 participants