Skip to content

gh repo edit#4318

Merged
mislav merged 2 commits intocli:trunkfrom
g14a:gh-repo-edit
Dec 10, 2021
Merged

gh repo edit#4318
mislav merged 2 commits intocli:trunkfrom
g14a:gh-repo-edit

Conversation

@g14a
Copy link
Contributor

@g14a g14a commented Sep 13, 2021

This PR adds a repo edit command to update settings of a repository.

Closes #995

@g14a
Copy link
Contributor Author

g14a commented Sep 13, 2021

@mislav I've implemented the repo edit command with just flags and no interactive menu like you mentioned in #3882. I've tested it from my end and it works fine. But I obviously need your input as well. Also let me know if I'm writing the tests the right way since this is a new command altogether :)

@g14a
Copy link
Contributor Author

g14a commented Sep 27, 2021

Hey @mislav just wanted to follow up a little on this. Thank you :)

@mislav mislav self-assigned this Oct 6, 2021
@g14a g14a requested a review from a team as a code owner October 22, 2021 16:32
@g14a g14a requested review from mislav and removed request for a team October 22, 2021 16:32
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 a good start!

The tests make sense; it looks like you have them set up properly.

I was confused by the difference in nomenclature between allow and enable flags -- if they are both toggles that accept true or false, they should probably be named similarly.

Some other stuff:

  • instead of -n, support -R as other commands do
  • avoid printing a success message if nothing was changed

One final consideration at this point is around renaming. We've added gh repo rename, so it's not strictly necessary that this command can rename; but it might feel weird to not support it. For now it's okay to punt on renaming in this command and we can worry about it later.

@g14a
Copy link
Contributor Author

g14a commented Oct 23, 2021

@vilmibm thank you for the feedback. I think we can go with enable for all the toggles i.e --enable-auto-merge etc. But how do I check if nothing has changed though? To avoid printing the error message

@vilmibm
Copy link
Contributor

vilmibm commented Oct 24, 2021

@g14a sounds good; as far as the nothing being changed, i just noticed that even with no flags passed to the command (gh repo edit) i got a success message. Ultimately this will be the interactive flow, so it's not an issue for now.

@g14a
Copy link
Contributor Author

g14a commented Oct 24, 2021

@vilmibm made the required changes. -n is a flag to rename the repo and not point to a repo, so -n and -R would be seperate in this case. Also I figured it would be better if the user knew what needs to be done in case no flag is set. So I added a FlagErrorWrap which says at least one flag is required. Let me know if anymore changes are needed :)

@g14a
Copy link
Contributor Author

g14a commented Nov 8, 2021

Hey @vilmibm just wanted to follow up regarding the changes.. thank you :)

@g14a
Copy link
Contributor Author

g14a commented Dec 2, 2021

@mislav @vilmibm just wanted to check if there's anything else to do here, thanks.

@mislav
Copy link
Contributor

mislav commented Dec 2, 2021

@g14a I'm looking over this now, will get back to you soon! Thanks for your work and your patience 🙇

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.

Hi @g14a, instead of requesting changes, I've pushed some simplifications to the branch.

  • The most important change is that the PATCH API request sent to update the repository will no longer contain all the parameters with their default values. Only explicitly edited fields are ever sent. This is accomplished by using pointer fields on the EditRepositoryInput struct and marking them as omitempty for JSON serialization. This avoids race conditions in repository updates, e.g. two different update operations that concurrently write to different fields on the same repository shouldn't reset each other's changes.
  • To make defining flags with pointer receivers easier, I've added helper functions for this to the cmdutil package.
  • A flag that wasn't passed at all will have its pointer value at nil. We can thus avoid excessive checks to cmd.Flags().Changed().
  • I've dropped the check that raises an at least one merge option must be true error since it didn't make sense in the context of this command.
  • I've also dropped the check for the validity of the --name flag. We let the server decide about the validity of values passed.
  • For now, I've removed the flag for changing the privacy/visibility of the repository. We have to first decide on an API for explicitly changing the visibility of the repository, and I would suggest avoiding boolean flags like --private. How about: --visibility={public,private,internal}?
  • For now, I've removed the flag to archive the repository since we have the gh repo archive command now.

Could you go over the changes to see if they make sense? Thank you! 🙇

@mislav mislav requested a review from samcoe December 6, 2021 15:00
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 looks really clean with the new flag utility functions!

@g14a
Copy link
Contributor Author

g14a commented Dec 7, 2021

@mislav damn! The new flag util functions look amazing and evreything looks much much cleaner! This feature was a roller coaster for me. Thank you :) Let's ship this to the moon 🚀🚀

g14a and others added 2 commits December 7, 2021 19:28
Co-authored-by: Mislav Marohnić <mislav@github.com>
@mislav
Copy link
Contributor

mislav commented Dec 7, 2021

Pushed updates:

  • Removed the ability to rename the repository via the --name parameter in favor of the existing gh repo rename command that has a bonus feature of also rewriting your local git remotes.
  • Added the ability to edit repo topics 🌟
  • Added the --visibility flag
  • Added the --allow-forking flag for org repos

@mislav mislav merged commit 2d0b946 into cli:trunk Dec 10, 2021
@Lilmomma42

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose more repository settings in repo create

5 participants