cli icon indicating copy to clipboard operation
cli copied to clipboard
verified publisher icon cli

add remote check to secret and variable commands

Open wingleung opened this issue 1 year ago • 5 comments

Fixes #4688

wingleung avatar May 14 '24 20:05 wingleung

@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

andyfeller avatar Jun 12 '24 13:06 andyfeller

Manual testing with forked repo

The 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.

  1. Building local gh for testing:

    andyfeller@Andys-MBP:cli/cli-trunk ‹add-remote-check-to-secret-and-variable›$ make
go build -trimpath -ldflags "-X github.com/cli/cli/v2/internal/build.Date=2024-08-23 -X github.com/cli/cli/v2/internal/build.Version=v2.55.0-13-g97b10370 " -o bin/gh ./cmd/gh

  2. Creating upstream repo for testing

    andyfeller@Andys-MBP:cli/cli-trunk ‹add-remote-check-to-secret-and-variable›$ ./bin/gh repo create --add-readme --description "Upstream repo used in testing GitHub CLI secret and variable behaviors" --disable-issues --disable-wiki --public andyfeller/gh-testing-01 
✓ Created repository andyfeller/gh-testing-01 on GitHub
  https://github.com/andyfeller/gh-testing-01

  3. Creating downstream fork for testing

    andyfeller@Andys-MBP:cli/cli-trunk ‹add-remote-check-to-secret-and-variable›$ cd ~/Documents/workspace/tinyfists 
andyfeller@Andys-MBP:workspace/tinyfists $ ~/Documents/workspace/cli/cli-trunk/bin/gh repo fork andyfeller/gh-testing-01 --org tinyfists --clone 
✓ Created fork tinyfists/gh-testing-01
Cloning into 'gh-testing-01'...
remote: Repository not found.
fatal: repository 'https://github.com/tinyfists/gh-testing-01.git/' not found
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.
From https://github.com/andyfeller/gh-testing-01
 * [new branch]      main       -> upstream/main
✓ Cloned fork
! Repository andyfeller/gh-testing-01 set as the default repository. To learn more about the default repository, run: gh repo set-default --help
andyfeller@Andys-MBP:workspace/tinyfists $ cd gh-testing-01

gh secret set

  1. Confirming repo with multiple remotes forces user to choose

    andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh secret set TEST001 --body "wut"
multiple remotes detected [upstream origin]. please specify which repo to use by providing the -R or --repo argument

  2. Confirming setting secret for upstream works

    andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh secret set TEST001 --body "wut" --repo tinyfists/gh-testing-01
✓ Set Actions secret TEST001 for tinyfists/gh-testing-01

  3. Confirming setting secret for downstream works

    andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh secret set TEST002 --body "wut" --repo andyfeller/gh-testing-01
✓ Set Actions secret TEST002 for andyfeller/gh-testing-01

gh secret list

  1. Confirming repo with multiple remotes forces user to choose

    andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh secret list                                                    
multiple remotes detected [upstream origin]. please specify which repo to use by providing the -R or --repo argument

  2. Confirming listing secret for upstream works

    andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh secret list --repo tinyfists/gh-testing-01
NAME     UPDATED               
TEST001  less than a minute ago

  3. Confirming listing secret for upstream works

    andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh secret list --repo andyfeller/gh-testing-01
NAME     UPDATED               
TEST002  less than a minute ago

gh secret delete

  1. Confirming repo with multiple remotes forces user to choose

    andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh secret delete TEST001
multiple remotes detected [upstream origin]. please specify which repo to use by providing the -R or --repo argument

  2. Confirming deleting non-existent secret for upstream works as expected

    andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh secret delete TEST001 --repo andyfeller/gh-testing-01
failed to delete secret TEST001: HTTP 404 (https://api.github.com/repos/andyfeller/gh-testing-01/actions/secrets/TEST001)

  3. Confirming deleting secret for downstream works as expected

    andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh secret delete TEST001 --repo tinyfists/gh-testing-01
✓ Deleted Actions secret TEST001 from tinyfists/gh-testing-01
andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$

gh variable set

  1. Confirming repo with multiple remotes forces user to choose
andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh variable set FOO --body "bar"
multiple remotes detected [upstream origin]. please specify which repo to use by providing the -R or --repo argument
  1. Confirming setting variable for upstream works as expected
andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh variable set FOO --body "bar" --repo tinyfists/gh-testing-01 
✓ Created variable FOO for tinyfists/gh-testing-01
  1. Confirming setting variable for downstream works as expected
andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh variable set FOO --body "baz" --repo andyfeller/gh-testing-01
✓ Created variable FOO for andyfeller/gh-testing-01

gh variable list

  1. Confirming repo with multiple remotes forces user to choose
andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh variable list                                                
multiple remotes detected [upstream origin]. please specify which repo to use by providing the -R or --repo argument
  1. Confirming listing variables for upstream works as expected
andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh variable list --repo tinyfists/gh-testing-01
NAME  VALUE  UPDATED               
FOO   bar    less than a minute ago
  1. Confirming listing variables for downstream works as expected
andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh variable list --repo andyfeller/gh-testing-01
NAME  VALUE  UPDATED               
FOO   baz    less than a minute ago

gh variable delete

  1. Confirming repo with multiple remotes forces user to choose
andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh variable delete FOO   
multiple remotes detected [upstream origin]. please specify which repo to use by providing the -R or --repo argument
  1. Confirming deleting variable for downstream works as expected
andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh variable delete FOO --repo tinyfists/gh-testing-01
✓ Deleted variable FOO from tinyfists/gh-testing-01

andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh variable list --repo tinyfists/gh-testing-01     
no variables found
  1. Confirming upstream variable still exists
andyfeller@Andys-MBP:tinyfists/gh-testing-01 ‹main›$ ~/Documents/workspace/cli/cli-trunk/bin/gh variable list --repo andyfeller/gh-testing-01
NAME  VALUE  UPDATED             
FOO   baz    about 28 minutes ago

andyfeller avatar Aug 23 '24 21:08 andyfeller

Manual testing directly against upstream

Wanting 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

andyfeller avatar Aug 23 '24 21:08 andyfeller

Just want to take a moment and say thank you for the monumental lift here, @wingleung ❤️

@andyfeller Pleasure was mine, especially because of the insights on high standards and the serene, collaborative environment we maintained throughout, love it 🙏

wingleung avatar Aug 25 '24 07:08 wingleung

@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

# Paste secret value for the current repository in an interactive prompt
$ gh secret set MYSECRET

# Read secret value from an environment variable
$ gh secret set MYSECRET --body "$ENV_VALUE"

I was under the impression gh secret set MYSECRET --body "$ENV_VALUE" is not an interactive prompt meaning you might use this command in use cases where prompts are not desired and we would rather want to throw an error and enforce a --repo flag where it helps make the use case more stable. For example for local scripts, CI, workflows

wingleung avatar Sep 24 '24 19:09 wingleung

@wingleung, thanks for you patience here, there's been a lot on.

The purpose was to only show prompts when prompts are desired, in this case we only show it when we're in interactive mode.

That's correct.

I was under the impression gh secret set MYSECRET --body "$ENV_VALUE" is not an interactive prompt meaning you might use this command in use cases where prompts are not desired and we would rather want to throw an error and enforce a --repo flag where it helps make the use case more stable. For example for local scripts, CI, workflows

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 gh, and whether there is a TTY attached. So yes, we require that in a non-interactive environment, all the necessary flags are provided. If we are in an interactive environment we have the ability to prompt the user. However, even with a TTY attached (and therefore interactivity enabled), I can still provide all the necessary flags to avoid prompting as is the case in:

gh secret set MYSECRET --body "$ENV_VALUE"

If I run this in my terminal, with more than one remote, gh has the capability to prompt, and it should. Otherwise we'd be forcing our interactive users to provide less information just so they can get the prompt.

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 main to reflect the changes, but I didn't want the variable changes to be included since it would be confusing. I also didn't want to push over your own work. Therefore I liberally reverted, squashed and rebased your commits into a history that should make sense for just your changes. I have ensured that you retain authorship for those commits as seen in https://github.com/cli/cli/pull/10209/commits

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 setRun function.

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 opts.HasRepoOverride into cmdutil.ValidateHasOnlyOneRemote, which is used like so:

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 --repo flag. This is a totally reasonable attempt to avoid duplication in other commands but I suggest that it's better to make this kind of decision early, because tracing the flag through the call stack to determine its impact requires holding context in your head. As a maintainer, I hope to avoid this kind of thing because well, unlike computers, we're pretty bad at remembering all the details.

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 setRun function doesn't need any knowledge of how a base repository was chosen, it just needs a way to get the base repo. In this case we have three variations of base repo selection behaviour:

  • Select the repo provided via the --repo flag
  • Select the repo from the git remote or error if there are more
  • Select the repo from a prompt if required

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 avatar Jan 21 '25 14:01 williammartin

@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 ❤️

wingleung avatar Feb 10 '25 13:02 wingleung