Skip to content
Navigation Menu
{{ message }}
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Require visibility confirmation in gh repo edit
#9845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
andyfeller
merged 2 commits into
trunk
from
andyfeller/9807-repo-edit-visibility-confirmation
Oct 30, 2024
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,63 @@ func TestNewCmdEdit(t *testing.T) { | |
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "deny public visibility change without accepting consequences", | ||
| args: "--visibility public", | ||
| wantOpts: EditOptions{ | ||
| Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), | ||
| Edits: EditRepositoryInput{}, | ||
| }, | ||
| wantErr: "use of --visibility flag requires --accept-visibility-change-consequences flag", | ||
| }, | ||
| { | ||
| name: "allow public visibility change with accepting consequences", | ||
| args: "--visibility public --accept-visibility-change-consequences", | ||
| wantOpts: EditOptions{ | ||
| Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), | ||
| Edits: EditRepositoryInput{ | ||
| Visibility: sp("public"), | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "deny private visibility change without accepting consequences", | ||
| args: "--visibility private", | ||
| wantOpts: EditOptions{ | ||
| Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), | ||
| Edits: EditRepositoryInput{}, | ||
| }, | ||
| wantErr: "use of --visibility flag requires --accept-visibility-change-consequences flag", | ||
| }, | ||
| { | ||
| name: "allow private visibility change with accepting consequences", | ||
| args: "--visibility private --accept-visibility-change-consequences", | ||
| wantOpts: EditOptions{ | ||
| Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), | ||
| Edits: EditRepositoryInput{ | ||
| Visibility: sp("private"), | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "deny internal visibility change without accepting consequences", | ||
| args: "--visibility internal", | ||
| wantOpts: EditOptions{ | ||
| Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), | ||
| Edits: EditRepositoryInput{}, | ||
| }, | ||
| wantErr: "use of --visibility flag requires --accept-visibility-change-consequences flag", | ||
| }, | ||
| { | ||
| name: "allow internal visibility change with accepting consequences", | ||
| args: "--visibility internal --accept-visibility-change-consequences", | ||
| wantOpts: EditOptions{ | ||
| Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), | ||
| Edits: EditRepositoryInput{ | ||
| Visibility: sp("internal"), | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
|
|
@@ -241,6 +298,109 @@ func Test_editRun_interactive(t *testing.T) { | |
| })) | ||
| }, | ||
| }, | ||
| { | ||
| name: "skipping visibility without confirmation", | ||
| opts: EditOptions{ | ||
| Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), | ||
| InteractiveMode: true, | ||
| }, | ||
| promptStubs: func(pm *prompter.MockPrompter) { | ||
| pm.RegisterMultiSelect("What do you want to edit?", nil, editList, | ||
| func(_ string, _, opts []string) ([]int, error) { | ||
| return []int{8}, nil | ||
| }) | ||
| pm.RegisterSelect("Visibility", []string{"public", "private", "internal"}, | ||
| func(_, _ string, opts []string) (int, error) { | ||
| return prompter.IndexFor(opts, "private") | ||
| }) | ||
| pm.RegisterConfirm("Do you want to change visibility to private?", func(_ string, _ bool) (bool, error) { | ||
| return false, nil | ||
| }) | ||
| }, | ||
| httpStubs: func(t *testing.T, reg *httpmock.Registry) { | ||
| reg.Register( | ||
| httpmock.GraphQL(`query RepositoryInfo\b`), | ||
| httpmock.StringResponse(` | ||
| { | ||
| "data": { | ||
| "repository": { | ||
| "visibility": "public", | ||
| "description": "description", | ||
| "homePageUrl": "https://url.com", | ||
| "defaultBranchRef": { | ||
| "name": "main" | ||
| }, | ||
| "stargazerCount": 10, | ||
| "isInOrganization": false, | ||
| "repositoryTopics": { | ||
| "nodes": [{ | ||
| "topic": { | ||
| "name": "x" | ||
| } | ||
| }] | ||
| } | ||
| } | ||
| } | ||
| }`)) | ||
| reg.Exclude(t, httpmock.REST("PATCH", "repos/OWNER/REPO")) | ||
| }, | ||
| wantsStderr: "Changing the repository visibility to private will cause permanent loss of 10 stars and 0 watchers.", | ||
| }, | ||
| { | ||
| name: "changing visibility with confirmation", | ||
| opts: EditOptions{ | ||
| Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), | ||
| InteractiveMode: true, | ||
| }, | ||
| promptStubs: func(pm *prompter.MockPrompter) { | ||
| pm.RegisterMultiSelect("What do you want to edit?", nil, editList, | ||
| func(_ string, _, opts []string) ([]int, error) { | ||
| return []int{8}, nil | ||
| }) | ||
| pm.RegisterSelect("Visibility", []string{"public", "private", "internal"}, | ||
| func(_, _ string, opts []string) (int, error) { | ||
| return prompter.IndexFor(opts, "private") | ||
| }) | ||
| pm.RegisterConfirm("Do you want to change visibility to private?", func(_ string, _ bool) (bool, error) { | ||
| return true, nil | ||
| }) | ||
| }, | ||
| httpStubs: func(t *testing.T, reg *httpmock.Registry) { | ||
| reg.Register( | ||
| httpmock.GraphQL(`query RepositoryInfo\b`), | ||
| httpmock.StringResponse(` | ||
| { | ||
| "data": { | ||
| "repository": { | ||
| "visibility": "public", | ||
| "description": "description", | ||
| "homePageUrl": "https://url.com", | ||
| "defaultBranchRef": { | ||
| "name": "main" | ||
| }, | ||
| "stargazerCount": 10, | ||
| "watchers": { | ||
| "totalCount": 15 | ||
| }, | ||
| "isInOrganization": false, | ||
| "repositoryTopics": { | ||
| "nodes": [{ | ||
| "topic": { | ||
| "name": "x" | ||
| } | ||
| }] | ||
| } | ||
| } | ||
| } | ||
| }`)) | ||
| reg.Register( | ||
| httpmock.REST("PATCH", "repos/OWNER/REPO"), | ||
| httpmock.RESTPayload(200, `{}`, func(payload map[string]interface{}) { | ||
| assert.Equal(t, "private", payload["visibility"]) | ||
| })) | ||
| }, | ||
| wantsStderr: "Changing the repository visibility to private will cause permanent loss of 10 stars and 15 watchers", | ||
| }, | ||
| { | ||
| name: "the rest", | ||
| opts: EditOptions{ | ||
|
|
@@ -250,7 +410,7 @@ func Test_editRun_interactive(t *testing.T) { | |
| promptStubs: func(pm *prompter.MockPrompter) { | ||
| pm.RegisterMultiSelect("What do you want to edit?", nil, editList, | ||
| func(_ string, _, opts []string) ([]int, error) { | ||
| return []int{0, 2, 3, 5, 6, 8, 9}, nil | ||
| return []int{0, 2, 3, 5, 6, 9}, nil | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a yikes (not your fault)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect the tight coupling of this code versus the code under test and the ordering of options is 🤢 which I agree isn't ideal. |
||
| }) | ||
| pm.RegisterInput("Default branch name", func(_, _ string) (string, error) { | ||
| return "trunk", nil | ||
|
|
@@ -267,13 +427,6 @@ func Test_editRun_interactive(t *testing.T) { | |
| pm.RegisterConfirm("Convert into a template repository?", func(_ string, _ bool) (bool, error) { | ||
| return true, nil | ||
| }) | ||
| pm.RegisterSelect("Visibility", []string{"public", "private", "internal"}, | ||
| func(_, _ string, opts []string) (int, error) { | ||
| return prompter.IndexFor(opts, "private") | ||
| }) | ||
| pm.RegisterConfirm("Do you want to change visibility to private?", func(_ string, _ bool) (bool, error) { | ||
| return true, nil | ||
| }) | ||
| pm.RegisterConfirm("Enable Wikis?", func(_ string, _ bool) (bool, error) { | ||
| return true, nil | ||
| }) | ||
|
|
@@ -310,7 +463,6 @@ func Test_editRun_interactive(t *testing.T) { | |
| assert.Equal(t, "https://zombo.com", payload["homepage"]) | ||
| assert.Equal(t, true, payload["has_issues"]) | ||
| assert.Equal(t, true, payload["has_projects"]) | ||
| assert.Equal(t, "private", payload["visibility"]) | ||
| assert.Equal(t, true, payload["is_template"]) | ||
| assert.Equal(t, true, payload["has_wiki"]) | ||
| })) | ||
|
|
@@ -484,7 +636,7 @@ func Test_editRun_interactive(t *testing.T) { | |
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| ios, _, _, _ := iostreams.Test() | ||
| ios, _, _, stderr := iostreams.Test() | ||
| ios.SetStdoutTTY(true) | ||
| ios.SetStdinTTY(true) | ||
| ios.SetStderrTTY(true) | ||
|
|
@@ -509,9 +661,11 @@ func Test_editRun_interactive(t *testing.T) { | |
| if tt.wantsErr == "" { | ||
| require.NoError(t, err) | ||
| } else { | ||
| assert.EqualError(t, err, tt.wantsErr) | ||
| require.EqualError(t, err, tt.wantsErr) | ||
| return | ||
| } | ||
|
|
||
| assert.Contains(t, stderr.String(), tt.wantsStderr) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
You can’t perform that action at this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering why you chose
ErrOuthere. When we have TTYs I always feel a bit like everything should go toOutunless it's an actual error, but it seems very gray. I'm never really sure though.In any case it's not a blocker and we can always change it later since this is interactive.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally feel how big of a gray area this is! 💯 Let me capture my thoughts but here is
cli/clicode use ofErrOut.Suffice to say, I see informational output like this similar to CLIG guidance as something that really isn't machine readable content but messaging / informational:
If someone was redirecting the output of this command somewhere, I wouldn't want this info to be swallowed up or treated differently if in non-TTY usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right I get all that generally, but this is only TTY interactive usage. You can't get to these new messages otherwise, right?