add remote check to secret and variable commands #9083

wingleung · 2024-05-14T20:21:55Z

…o-secret-and-variable

pkg/cmd/secret/delete/delete.go

andyfeller · 2024-06-12T13:35:06Z

@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

pkg/cmdutil/errors.go

…o-secret-and-variable

pkg/cmdutil/repo_override.go

…o-secret-and-variable

andyfeller · 2024-08-16T17:55:07Z

pkg/cmd/variable/set/set.go

+
+		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
+			}
+		}
+


Taking a step back from the code, is the following the logical order that repository should be identified?

If a user specifies --owner this is an organization secret; otherwise it is a repository secret of some sort

If a user specifies --repo this 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

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-default as this is technically the problem we're running into

This PR should explicitly call out in gh repo set-default that 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

If a user specifies --owner this 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 --repo this 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

During our conversation, Will highlighted several things including:

Remove the line about gh repo set-default as 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-default that 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 👉

cli/context/remote.go

Lines 60 to 78 in 95a2f95

func remoteNameSortScore(name string) int {

switch strings.ToLower(name) {

case "upstream":

return 3

case "github":

return 2

case "origin":

return 1

default:

return 0

}

}

// https://golang.org/pkg/sort/#Interface

func (r Remotes) Len() int { return len(r) }

func (r Remotes) Swap(i, j int) { r[i], r[j] = r[j], r[i] }

func (r Remotes) Less(i, j int) bool {

return remoteNameSortScore(r[i].Name) > remoteNameSortScore(r[j].Name)

}

so the order is always 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

andyfeller · 2024-08-23T21:37:36Z

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.

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

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

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`

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

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

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`

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

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

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`

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

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)

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`

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

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

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`

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

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

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`

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

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

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 · 2024-08-23T21:48:01Z

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

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

wingleung · 2024-08-25T07:01:13Z

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 🙏

williammartin

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

wingleung added 2 commits May 14, 2024 22:18

WIP add remote check to secret and variable commands

c80ba70

add tests

5364a0c

wingleung marked this pull request as ready for review June 2, 2024 10:28

wingleung requested a review from a team as a code owner June 2, 2024 10:28

wingleung requested a review from andyfeller June 2, 2024 10:28

cliAutomation added the external pull request originating outside of the CLI core team label Jun 2, 2024

Merge remote-tracking branch 'upstream/trunk' into add-remote-check-t…

29ad2b6

…o-secret-and-variable

Jorslope reviewed Jun 2, 2024

View reviewed changes

pkg/cmd/secret/delete/delete.go Show resolved Hide resolved

wingleung changed the title ~~WIP add remote check to secret and variable commands~~ add remote check to secret and variable commands Jun 3, 2024

andyfeller reviewed Jun 12, 2024

View reviewed changes

pkg/cmdutil/errors.go Show resolved Hide resolved

wingleung added 3 commits August 12, 2024 15:05

Merge remote-tracking branch 'upstream/trunk' into add-remote-check-t…

1d03581

…o-secret-and-variable

prompt to select a repo in interactive mode

81e7ecd

fix lint

df723bd

andyfeller reviewed Aug 12, 2024

View reviewed changes

pkg/cmdutil/repo_override.go Show resolved Hide resolved

wingleung added 3 commits August 16, 2024 09:04

use default set remote if available

5798eb7

Merge remote-tracking branch 'upstream/trunk' into add-remote-check-t…

c4e3204

…o-secret-and-variable

add todos for tests

01f1935

andyfeller reviewed Aug 16, 2024

View reviewed changes

wingleung added 4 commits August 21, 2024 17:07

Revert "use default set remote if available"

5212f12

This reverts commit 5798eb7.

update docs for set-default

0f9ad3b

NOTE: gh does not use the default repository for managing repository and environment secrets or variables.

Merge remote-tracking branch 'upstream/trunk' into add-remote-check-t…

4854efb

…o-secret-and-variable

update setdefault test

97b1037

andyfeller approved these changes Aug 23, 2024

View reviewed changes

andyfeller self-requested a review September 18, 2024 12:27

williammartin requested changes Sep 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add remote check to secret and variable commands #9083

add remote check to secret and variable commands #9083

wingleung commented May 14, 2024

andyfeller commented Jun 12, 2024

andyfeller Aug 16, 2024 •

edited

Loading

andyfeller Aug 19, 2024 •

edited

Loading

wingleung Aug 21, 2024 •

edited

Loading

wingleung Aug 21, 2024 •

edited

Loading

andyfeller commented Aug 23, 2024

andyfeller commented Aug 23, 2024

andyfeller left a comment

wingleung commented Aug 25, 2024 •

edited

Loading

williammartin left a comment •

edited

Loading

	func remoteNameSortScore(name string) int {
	switch strings.ToLower(name) {
	case "upstream":
	return 3
	case "github":
	return 2
	case "origin":
	return 1
	default:
	return 0
	}
	}

	// https://golang.org/pkg/sort/#Interface
	func (r Remotes) Len() int { return len(r) }
	func (r Remotes) Swap(i, j int) { r[i], r[j] = r[j], r[i] }
	func (r Remotes) Less(i, j int) bool {
	return remoteNameSortScore(r[i].Name) > remoteNameSortScore(r[j].Name)
	}

add remote check to secret and variable commands #9083

Are you sure you want to change the base?

add remote check to secret and variable commands #9083

Conversation

wingleung commented May 14, 2024

andyfeller commented Jun 12, 2024

andyfeller Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

andyfeller Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Discussing with @williammartin ✨

wingleung Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

wingleung Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

andyfeller commented Aug 23, 2024

Manual testing with forked repo

gh secret set

gh secret list

gh secret delete

gh variable set

gh variable list

gh variable delete

andyfeller commented Aug 23, 2024

Manual testing directly against upstream

andyfeller left a comment

Choose a reason for hiding this comment

wingleung commented Aug 25, 2024 • edited Loading

williammartin left a comment • edited Loading

Choose a reason for hiding this comment

Acceptance Scripts

andyfeller Aug 16, 2024 •

edited

Loading

andyfeller Aug 19, 2024 •

edited

Loading

wingleung Aug 21, 2024 •

edited

Loading

wingleung Aug 21, 2024 •

edited

Loading

`gh secret set`

`gh secret list`

`gh secret delete`

`gh variable set`

`gh variable list`

`gh variable delete`

wingleung commented Aug 25, 2024 •

edited

Loading

williammartin left a comment •

edited

Loading