add remote check to secret and variable commands #9083
Conversation
cliAutomation
added
the
external
…o-secret-and-variable
wingleung
changed the title
|
@wingleung : thank you yet again for your time and effort to improve the GitHub CLI and your patience with my slow follow up! 🙇 reviewing this right now, wanting to offer feedback today |
|
|
||
| err = cmdutil.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes) | ||
| if err != nil { | ||
| defaultRepo := cmdutil.DefaultRepo(opts.Remotes) | ||
|
|
||
| if defaultRepo != nil { | ||
| baseRepo = defaultRepo | ||
| } else if opts.Interactive { | ||
| selectedRepo, errSelectedRepo := cmdutil.PromptForRepo(baseRepo, opts.Remotes, opts.Prompter) | ||
|
|
||
| if errSelectedRepo != nil { | ||
| return errSelectedRepo | ||
| } | ||
|
|
||
| baseRepo = selectedRepo | ||
| } else { | ||
| return err | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Taking a step back from the code, is the following the logical order that repository should be identified?
-
If a user specifies
--ownerthis is an organization secret; otherwise it is a repository secret of some sort -
If a user specifies
--repothis has the highest precedence because the user is explicitly telling us what to use -
Otherwise, we figure out the repo based on the remotes, dealing with various scenarios
- If there are multiple remotes but we cannot interact, error
- If there are multiple remotes and we can interact, prompt the user and maybe mention they can avoid this in the future with either using
--repo ... - If there is 1 remote, we use it
There was a problem hiding this comment.
Discussing with @williammartin ✨
@wingleung : firstly, allow me to apologize for confusion on my part; Will has highlighted the fact that gh.resolved flag on the remote that provides the base repo is actually contributing to the challenge we're trying to undo here.
During our conversation, Will highlighted several things including:
- Remove the line about
gh repo set-defaultas this is technically the problem we're running into - This PR should explicitly call out in
gh repo set-defaultthat it IS NOT used for repository + repository environment - In the remote prompt ordering, the upstream should be first as we know upset users will want to hit enter
- With these changes, we need to ensure it is called out in release notes and mention the community should follow up in this issue about interest in a configuration option
- Will also called out a larger problem regarding the inconsistent handling of repository defaults that might necessitate further changes
There was a problem hiding this comment.
- If a user specifies
--ownerthis is an organization secret; otherwise it is a repository secret of some sort
I think you meant --org instead of --owner, if so ✅
- If a user specifies
--repothis has the highest precedence because the user is explicitly telling us what to use
I think number 2 would be a secret or variable set with the --env flag, to set variables and secrets on a deployment environment level. which would be place between --org and --repo in terms of scope level
There was a problem hiding this comment.
During our conversation, Will highlighted several things including:
- Remove the line about
gh repo set-defaultas this is technically the problem we're running into
OK, reverted the last commit, it will still select the default in the select prompt if there is a default but submitting is up to the user
- This PR should explicitly call out in
gh repo set-defaultthat it IS NOT used for repository + repository environment
Idd, it would do the exact opposite to what's in the docs, so the guidelines for gh repo set-default should change as well, added a proposal ### NOTE: gh does not use the default repository for managing repository and environment secrets or variables.. Let me know if you see any other places where we can add this note
- In the remote prompt ordering, the upstream should be first as we know upset users will want to hit enter
We piggyback on Remotes which is doing sorting by itself using the Less function and the sort library from golang 👉
Lines 60 to 78 in 95a2f95
upstream first even though the default selected option might be the origin one. Or do you mean we should always select the first option which is the upstream option? That would be not be as intuitive because I would expect the origin to be the default option
- With these changes, we need to ensure it is called out in release notes and mention the community should follow up in this issue about interest in a configuration option
👍
- Will also called out a larger problem regarding the inconsistent handling of repository defaults that might necessitate further changes
I agree, I also mentioned something similar here 👉 #9083 (comment). problem is, it's not just a feature or a change to a command, it's refactoring with a higher level overview in mind. Is that something I can kickstart in https://github.com/cli/cli/discussions/categories/ideas? Or do you prefer to have this discussion internally first?
This reverts commit 5798eb7.
NOTE: gh does not use the default repository for managing repository and environment secrets or variables.
…o-secret-and-variable
Manual testing with forked repoThe following scenarios are exercising changes from the PR on a fork of a repo, which should require explicitly setting the repo because of multiple remotes.
|
Manual testing directly against upstreamWanting to make sure we were just testing the multiple remotes scenario, this is me running through several of the commands to make sure no regressions. andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ cd ../../andyfeller
andyfeller@Andys-MBP:workspace/andyfeller $ gh repo clone gh-testing-01
Cloning into 'gh-testing-01'...
remote: Enumerating objects: 3, done.
remote: Counting objects: 100% (3/3), done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
Receiving objects: 100% (3/3), done.
andyfeller@Andys-MBP:workspace/andyfeller $ cd gh-testing-01
andyfeller@Andys-MBP:andyfeller/gh-testing-01 ‹main›$ gh variable list
NAME VALUE UPDATED
FOO baz about 1 hour ago
andyfeller@Andys-MBP:andyfeller/gh-testing-01 ‹main›$ gh secret list
NAME UPDATED
TEST002 about 1 hour ago
andyfeller@Andys-MBP:andyfeller/gh-testing-01 ‹main›$ gh secret set TEST003 --body "on the catwalk"
✓ Set Actions secret TEST003 for andyfeller/gh-testing-01
andyfeller@Andys-MBP:andyfeller/gh-testing-01 ‹main›$ gh secret list
NAME UPDATED
TEST002 about 1 hour ago
TEST003 less than a minute ago
andyfeller@Andys-MBP:andyfeller/gh-testing-01 ‹main›$ gh variable set BAR --body "pipe"
✓ Created variable BAR for andyfeller/gh-testing-01
andyfeller@Andys-MBP:andyfeller/gh-testing-01 ‹main›$ gh variable list
NAME VALUE UPDATED
BAR pipe less than a minute ago
FOO baz about 1 hour ago
andyfeller@Andys-MBP:andyfeller/gh-testing-01 ‹main›$ gh variable get BAR
pipe
andyfeller@Andys-MBP:andyfeller/gh-testing-01 ‹main›$ gh variable delete FOO
✓ Deleted variable FOO from andyfeller/gh-testing-01
andyfeller@Andys-MBP:andyfeller/gh-testing-01 ‹main›$ gh secret delete TEST002
✓ Deleted Actions secret TEST002 from andyfeller/gh-testing-01 |
There was a problem hiding this comment.
Just want to take a moment and say thank you for the monumental lift here, @wingleung ❤️
From the automated tests here and the manual testing I commented on in the PR, I think this might be complete, so I'm giving my tentative 
I'd like @williammartin to follow up when he's back on Wednesday next week solely because of how critical getting this right is. 🙇
Promise its in the home stretch; again thank you for your patience and closing a security concern in the GitHub CLI! ❤️
@andyfeller Pleasure was mine, especially because of the insights on high standards and the serene, collaborative environment we maintained throughout, love it 🙏 |
There was a problem hiding this comment.
I think there is some surprising behaviour in this PR as it is. In particular, I would expect the following three scenarios to prompt:
gh repo create my-org/my-repo
gh repo fork --org other-org my-org/my-repo
gh repo clone other-org/my-repo
cd my-repo
gh secret set VAL --body KEY
gh repo create my-org/my-repo
gh repo fork --org other-org my-org/my-repo
gh repo clone other-org/my-repo
cd my-repo
gh secret list
gh repo create my-org/my-repo
gh repo fork --org other-org my-org/my-repo
gh repo clone other-org/my-repo
cd my-repo
gh secret delete VAL
However, they do not right now, and I think this was alluded to here #9083 (comment)
Erroring and forcing users to provide a --repo flag in an interactive environment in some cases but not others seems inconsistent.
Additionally, I'm not convinced we should be modifying the variable set of commands. Breaking people's scripts for the sake of security is one thing, breaking them for consistent UX is another. Fortunately this should be easy to back out / and do later if we determine it is worthwhile.
We're going to discuss in our planning meeting later how to get this in. We feel bad that it's been languishing for so long (and I feel bad because Andy has been asking for my review for weeks 😬 ), so just hang in there @wingleung !
Acceptance Scripts
Based on #4688 (comment) I created a set of acceptance scripts over in 0c9b6ed
|
@williammartin thanks for the review! The purpose was to only show prompts when prompts are desired, in this case we only show it when we're in interactive mode. Maybe I'm misinterpreting the following documentation I was under the impression |
|
@wingleung, thanks for you patience here, there's been a lot on.
That's correct.
But this isn't quite correct. The fundamental misunderstanding is that interactivity is not a property of the flags, but of the environment that is invoking If I run this in my terminal, with more than one remote, I went round and round a bit on how I wanted to address this and the code changes and in the end I decided to create a new PR to replace this one. This is because I wanted the git history in I also want to make sure to give you some feedback on your code, and why I changed the parts I changed. In particular, I want to focus on the following code, which was positioned within the err = cmdutil.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes)
if err != nil {
if opts.Interactive {
selectedRepo, errSelectedRepo := cmdutil.PromptForRepo(baseRepo, opts.Remotes, opts.Prompter)
if errSelectedRepo != nil {
return errSelectedRepo
}
baseRepo = selectedRepo
} else {
return err
}
}This code is not bad at all; in particular it is clear to read from top to bottom. However, in my experience, this kind of procedural code over time often becomes a maintenance burden because the variation in behaviour becomes a series of conditionals (sometimes nested) that accrete over time. One of the biggest indicators to me that there might be a better pattern was the injection of func ValidateHasOnlyOneRemote(hasRepoOverride bool, remotes func() (ghContext.Remotes, error)) error {
if !hasRepoOverride && remotes != nil {
...
}
return nil
}I assume this was an attempt to provide a shared location between commands to shortcircuit the validation if the user had provided the Instead of having deeply nested conditonals, I look to push the conditionals back up the stack. Sandi Metz (I strongly recommend this video) would describe my chosen approach as isolating the behaviour we want to vary, creating things to fulfil those differing behaviours, and injecting the correct smarter thing. Concretely, the
With this in mind we are able to push the conditional decision up the stack: opts.BaseRepo = f.BaseRepo
if !cmd.Flags().Changed("repo") {
// If they haven't specified a repo directly, then we will wrap the BaseRepoFunc in one that errors if
// there might be multiple valid remotes.
opts.BaseRepo = shared.RequireNoAmbiguityBaseRepoFunc(opts.BaseRepo, f.Remotes)
// But if we are able to prompt, then we will wrap that up in a BaseRepoFunc that can prompt the user to
// resolve the ambiguity.
if opts.IO.CanPrompt() {
opts.BaseRepo = shared.PromptWhenAmbiguousBaseRepoFunc(opts.BaseRepo, f.IOStreams, f.Prompter)
}
}We could go further and push this conditional into a single shared location to be used by each command, but I don't believe it provides significantly more value. Furthermore this provides us with some nice testable units that don't require working through the entire command invocation. Again, apologise for such a long wait on this one. I hope you don't mind that I opened a new PR to get this through, I believe that you have been properly credited for your work in the commits. Thank you for kicking this off and giving us some code to work with, otherwise I think this wouldn't have got any attention for much longer than it already had. Cheers, Will |
|
@williammartin I want to thank you for the feedback on this PR, it's very clear and gave me a bit more to learn from (Sandi Metz Talk 👍 ). Love your refactor of keeping the downstream functions simple and context agnostic by extracting the parameters in the entrypoint of a command ❤️ |
5 participants
Fixes #4688