From 161ad92b20631ee8e367b59d0018ccfbf4dc3c9d Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Sun, 9 Feb 2025 21:58:15 +0500 Subject: [PATCH 01/25] [gh release create/upload] Expand glob patterns on Windows --- pkg/cmd/release/shared/upload.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 38add7425..10da2e556 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -4,12 +4,15 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "mime" "net/http" "net/url" "os" "path" + "path/filepath" + "runtime" "strings" "time" @@ -36,6 +39,12 @@ type AssetForUpload struct { } func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { + if runtime.GOOS == "windows" { + args, err = globAssetPatterns(args) + if err != nil { + return nil, err + } + } for _, arg := range args { var label string fn := arg @@ -63,6 +72,22 @@ func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { return } +func globAssetPatterns(patterns []string) ([]string, error) { + var assets []string + for _, pattern := range patterns { + matches, err := filepath.Glob(pattern) + if err != nil { + return nil, fmt.Errorf("%s: %s", pattern, err) + } + if len(matches) > 0 { + assets = append(assets, matches...) + } else { + assets = append(assets, pattern) + } + } + return assets, nil +} + func typeForFilename(fn string) string { ext := fileExt(fn) switch ext { From d9bfa606ccce89fce82b5a7809423cd6157a7838 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Tue, 11 Feb 2025 12:56:32 +0500 Subject: [PATCH 02/25] Fix `gh gist create` for multiple filenames and glob patterns --- pkg/cmd/gist/create/create.go | 14 +++++++++----- pkg/cmd/release/shared/upload.go | 28 ++++------------------------ pkg/cmdutil/args.go | 21 +++++++++++++++++++++ 3 files changed, 34 insertions(+), 29 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index 4403df9be..ad697e2ce 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -85,7 +85,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co }, Aliases: []string{"new"}, RunE: func(c *cobra.Command, args []string) error { - opts.Filenames = args + opts.Filenames = args[:] if runF != nil { return runF(&opts) @@ -102,12 +102,16 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } func createRun(opts *CreateOptions) error { - fileArgs := opts.Filenames - if len(fileArgs) == 0 { - fileArgs = []string{"-"} + filenames, err := cmdutil.GlobWindowsPaths(opts.Filenames) + if err != nil { + return err } - files, err := processFiles(opts.IO.In, opts.FilenameOverride, fileArgs) + if len(filenames) == 0 { + filenames = []string{"-"} + } + + files, err := processFiles(opts.IO.In, opts.FilenameOverride, filenames) if err != nil { return fmt.Errorf("failed to collect files for posting: %w", err) } diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 10da2e556..159f86e23 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -4,20 +4,18 @@ import ( "context" "encoding/json" "errors" - "fmt" "io" "mime" "net/http" "net/url" "os" "path" - "path/filepath" - "runtime" "strings" "time" "github.com/cenkalti/backoff/v4" "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/pkg/cmdutil" "golang.org/x/sync/errgroup" ) @@ -39,11 +37,9 @@ type AssetForUpload struct { } func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { - if runtime.GOOS == "windows" { - args, err = globAssetPatterns(args) - if err != nil { - return nil, err - } + args, err = cmdutil.GlobWindowsPaths(args) + if err != nil { + return nil, err } for _, arg := range args { var label string @@ -72,22 +68,6 @@ func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { return } -func globAssetPatterns(patterns []string) ([]string, error) { - var assets []string - for _, pattern := range patterns { - matches, err := filepath.Glob(pattern) - if err != nil { - return nil, fmt.Errorf("%s: %s", pattern, err) - } - if len(matches) > 0 { - assets = append(assets, matches...) - } else { - assets = append(assets, pattern) - } - } - return assets, nil -} - func typeForFilename(fn string) string { ext := fileExt(fn) switch ext { diff --git a/pkg/cmdutil/args.go b/pkg/cmdutil/args.go index 0f03a07bd..9dfbce2b1 100644 --- a/pkg/cmdutil/args.go +++ b/pkg/cmdutil/args.go @@ -2,6 +2,8 @@ package cmdutil import ( "fmt" + "path/filepath" + "runtime" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -57,3 +59,22 @@ func NoArgsQuoteReminder(cmd *cobra.Command, args []string) error { return FlagErrorf("%s", errMsg) } + +func GlobWindowsPaths(patterns []string) ([]string, error) { + if runtime.GOOS != "windows" { + return patterns, nil + } + var expansions []string + for _, pattern := range patterns { + matches, err := filepath.Glob(pattern) + if err != nil { + return nil, fmt.Errorf("%s: %s", pattern, err) + } + if len(matches) > 0 { + expansions = append(expansions, matches...) + } else { + expansions = append(expansions, pattern) + } + } + return expansions, nil +} From 8748bb0b1a758b2fa940014762d18a191a9e603c Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 21 Feb 2025 09:33:19 -0800 Subject: [PATCH 03/25] Add testing to GlobWindowsPaths (#2) --- pkg/cmd/gist/create/create.go | 11 ++- pkg/cmd/release/shared/upload.go | 9 +- pkg/cmdutil/args.go | 10 +-- pkg/cmdutil/args_test.go | 150 ++++++++++++++++++++++++++++++- 4 files changed, 167 insertions(+), 13 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index ad697e2ce..b186524c5 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "regexp" + "runtime" "sort" "strings" @@ -102,9 +103,13 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } func createRun(opts *CreateOptions) error { - filenames, err := cmdutil.GlobWindowsPaths(opts.Filenames) - if err != nil { - return err + filenames := opts.Filenames + if runtime.GOOS == "windows" { + globbedNames, err := cmdutil.GlobWindowsPaths(opts.Filenames) + if err != nil { + return err + } + filenames = globbedNames } if len(filenames) == 0 { diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 159f86e23..1f3570b59 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -10,6 +10,7 @@ import ( "net/url" "os" "path" + "runtime" "strings" "time" @@ -37,9 +38,11 @@ type AssetForUpload struct { } func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { - args, err = cmdutil.GlobWindowsPaths(args) - if err != nil { - return nil, err + if runtime.GOOS == "windows" { + args, err = cmdutil.GlobWindowsPaths(args) + if err != nil { + return nil, err + } } for _, arg := range args { var label string diff --git a/pkg/cmdutil/args.go b/pkg/cmdutil/args.go index 9dfbce2b1..196621b82 100644 --- a/pkg/cmdutil/args.go +++ b/pkg/cmdutil/args.go @@ -3,7 +3,6 @@ package cmdutil import ( "fmt" "path/filepath" - "runtime" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -61,14 +60,12 @@ func NoArgsQuoteReminder(cmd *cobra.Command, args []string) error { } func GlobWindowsPaths(patterns []string) ([]string, error) { - if runtime.GOOS != "windows" { - return patterns, nil - } - var expansions []string + expansions := []string{} + for _, pattern := range patterns { matches, err := filepath.Glob(pattern) if err != nil { - return nil, fmt.Errorf("%s: %s", pattern, err) + return nil, fmt.Errorf("%s: %v", pattern, err) } if len(matches) > 0 { expansions = append(expansions, matches...) @@ -76,5 +73,6 @@ func GlobWindowsPaths(patterns []string) ([]string, error) { expansions = append(expansions, pattern) } } + return expansions, nil } diff --git a/pkg/cmdutil/args_test.go b/pkg/cmdutil/args_test.go index db0b96510..60a37c295 100644 --- a/pkg/cmdutil/args_test.go +++ b/pkg/cmdutil/args_test.go @@ -1,6 +1,13 @@ package cmdutil -import "testing" +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) func TestMinimumArgs(t *testing.T) { tests := []struct { @@ -48,3 +55,144 @@ func TestMinimumNs_with_error(t *testing.T) { } } } + +func TestGlobWindowsPaths(t *testing.T) { + tests := []struct { + name string + os string + patterns []string + wantOut []string + wantErr error + }{ + { + name: "When no patterns are passed, return an empty slice", + patterns: []string{}, + wantOut: []string{}, + wantErr: nil, + }, + { + name: "When no files match, it returns an empty expansions array, it returns the unmatched patterns", + patterns: []string{"foo", "bar"}, + wantOut: []string{"foo", "bar"}, + wantErr: nil, + }, + { + name: "When a single pattern, '*.txt' is passed with one match, it returns that match", + patterns: []string{ + "*.txt", + }, + wantOut: []string{ + "rootFile.txt", + }, + wantErr: nil, + }, + { + name: "When a single pattern, '*/*.txt' is passed with multiple matches, it returns those matches", + patterns: []string{ + "*/*.txt", + }, + wantOut: []string{ + "subDir1/subDir1_file.txt", + "subDir2/subDir2_file.txt", + }, + wantErr: nil, + }, + { + name: "When multiple patterns, '*/*.txt' and '*/*.go', are passed with multiple matches, it returns those matches", + patterns: []string{ + "*/*.txt", + "*/*.go", + }, + wantOut: []string{ + "subDir1/subDir1_file.txt", + "subDir2/subDir2_file.txt", + "subDir2/subDir2_file.go", + }, + wantErr: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cleanupFn := createTestDir(t) + defer cleanupFn() + + got, err := GlobWindowsPaths(tt.patterns) + if tt.wantErr != nil { + assert.EqualError(t, err, tt.wantErr.Error()) + } else { + require.NoError(t, err) + } + assert.Equal(t, tt.wantOut, got) + }) + } +} + +// Creates a temporary directory with the structure below. Returns +// a cleanup function that will remove the directory and all of its +// contents. The cleanup function should be wrapped in a defer statement. +// +// | root +// |-- rootFile.txt +// |-- subDir1 +// | |-- subDir1_file.txt +// | +// |-- subDir2 +// |-- subDir2_file.go +// |-- subDir2_file.txt +func createTestDir(t *testing.T) (cleanupFn func()) { + t.Helper() + // Make Directories + rootDir := t.TempDir() + + // Move workspace to temporary directory + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + err = os.Chdir(rootDir) + if err != nil { + t.Fatal(err) + } + + // Make subdirectories + err = os.Mkdir(filepath.Join(rootDir, "subDir1"), 0755) + if err != nil { + t.Fatal(err) + } + + err = os.Mkdir(filepath.Join(rootDir, "subDir2"), 0755) + if err != nil { + t.Fatal(err) + } + + // Make Files + err = os.WriteFile(filepath.Join(rootDir, "rootFile.txt"), []byte(""), 0644) + if err != nil { + t.Fatal(err) + } + + err = os.WriteFile(filepath.Join(rootDir, "subDir1", "subDir1_file.txt"), []byte(""), 0o644) + if err != nil { + t.Fatal(err) + } + + err = os.WriteFile(filepath.Join(rootDir, "subDir2", "subDir2_file.go"), []byte(""), 0o644) + if err != nil { + t.Fatal(err) + } + + err = os.WriteFile(filepath.Join(rootDir, "subDir2", "subDir2_file.txt"), []byte(""), 0o644) + if err != nil { + t.Fatal(err) + } + + cleanupFn = func() { + os.RemoveAll(rootDir) + err = os.Chdir(cwd) + if err != nil { + t.Fatal(err) + } + } + return cleanupFn +} From 092eb7e962799db70bc0925fc9684eee0f6f0a30 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Sat, 22 Feb 2025 12:16:42 +0500 Subject: [PATCH 04/25] Support globbing for all platforms --- pkg/cmd/gist/create/create.go | 18 ++++++++---------- pkg/cmd/release/create/create.go | 2 +- pkg/cmd/release/shared/upload.go | 9 +++------ pkg/cmdutil/args.go | 20 ++++++++++++-------- pkg/cmdutil/args_test.go | 21 +++++++++++---------- 5 files changed, 35 insertions(+), 35 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index b186524c5..3eda8b0bd 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -10,7 +10,6 @@ import ( "os" "path/filepath" "regexp" - "runtime" "sort" "strings" @@ -49,13 +48,13 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } cmd := &cobra.Command{ - Use: "create [... | -]", + Use: "create [... | ... | -]", Short: "Create a new gist", Long: heredoc.Docf(` Create a new GitHub gist with given contents. Gists can be created from one or multiple files. Alternatively, pass %[1]s-%[1]s as - file name to read from standard input. + filename to read from standard input. By default, gists are secret; use %[1]s--public%[1]s to make publicly listed ones. `, "`"), @@ -69,6 +68,9 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co # create a gist containing several files $ gh gist create hello.py world.py cool.txt + # create a gist containing several files using patterns + $ gh gist create *.md *.txt artifact.* + # read from standard input to create a gist $ gh gist create - @@ -103,13 +105,9 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } func createRun(opts *CreateOptions) error { - filenames := opts.Filenames - if runtime.GOOS == "windows" { - globbedNames, err := cmdutil.GlobWindowsPaths(opts.Filenames) - if err != nil { - return err - } - filenames = globbedNames + filenames, err := cmdutil.GlobPaths(opts.Filenames) + if err != nil { + return err } if len(filenames) == 0 { diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index 8e77546d0..dea41dd57 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -77,7 +77,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co cmd := &cobra.Command{ DisableFlagsInUseLine: true, - Use: "create [] [...]", + Use: "create [] [... | ...]", Short: "Create a new release", Long: heredoc.Docf(` Create a new GitHub Release for a repository. diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 1f3570b59..0b6468c3c 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -10,7 +10,6 @@ import ( "net/url" "os" "path" - "runtime" "strings" "time" @@ -38,11 +37,9 @@ type AssetForUpload struct { } func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { - if runtime.GOOS == "windows" { - args, err = cmdutil.GlobWindowsPaths(args) - if err != nil { - return nil, err - } + args, err = cmdutil.GlobPaths(args) + if err != nil { + return nil, err } for _, arg := range args { var label string diff --git a/pkg/cmdutil/args.go b/pkg/cmdutil/args.go index 196621b82..6bbe8c300 100644 --- a/pkg/cmdutil/args.go +++ b/pkg/cmdutil/args.go @@ -3,6 +3,7 @@ package cmdutil import ( "fmt" "path/filepath" + "strings" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -59,18 +60,21 @@ func NoArgsQuoteReminder(cmd *cobra.Command, args []string) error { return FlagErrorf("%s", errMsg) } -func GlobWindowsPaths(patterns []string) ([]string, error) { +func GlobPaths(patterns []string) ([]string, error) { expansions := []string{} for _, pattern := range patterns { - matches, err := filepath.Glob(pattern) - if err != nil { - return nil, fmt.Errorf("%s: %v", pattern, err) - } - if len(matches) > 0 { - expansions = append(expansions, matches...) - } else { + if pattern == "-" || strings.Contains(pattern, "#") { expansions = append(expansions, pattern) + } else { + matches, err := filepath.Glob(pattern) + if err != nil { + return nil, fmt.Errorf("%s: %v", pattern, err) + } + if len(matches) == 0 { + return []string{}, fmt.Errorf("no matches found for `%s`", pattern) + } + expansions = append(expansions, matches...) } } diff --git a/pkg/cmdutil/args_test.go b/pkg/cmdutil/args_test.go index 60a37c295..c2602bbe9 100644 --- a/pkg/cmdutil/args_test.go +++ b/pkg/cmdutil/args_test.go @@ -1,6 +1,7 @@ package cmdutil import ( + "errors" "os" "path/filepath" "testing" @@ -56,7 +57,7 @@ func TestMinimumNs_with_error(t *testing.T) { } } -func TestGlobWindowsPaths(t *testing.T) { +func TestGlobPaths(t *testing.T) { tests := []struct { name string os string @@ -72,9 +73,9 @@ func TestGlobWindowsPaths(t *testing.T) { }, { name: "When no files match, it returns an empty expansions array, it returns the unmatched patterns", - patterns: []string{"foo", "bar"}, - wantOut: []string{"foo", "bar"}, - wantErr: nil, + patterns: []string{"foo"}, + wantOut: []string{}, + wantErr: errors.New("no matches found for `foo`"), }, { name: "When a single pattern, '*.txt' is passed with one match, it returns that match", @@ -92,8 +93,8 @@ func TestGlobWindowsPaths(t *testing.T) { "*/*.txt", }, wantOut: []string{ - "subDir1/subDir1_file.txt", - "subDir2/subDir2_file.txt", + filepath.Join("subDir1", "subDir1_file.txt"), + filepath.Join("subDir2", "subDir2_file.txt"), }, wantErr: nil, }, @@ -104,9 +105,9 @@ func TestGlobWindowsPaths(t *testing.T) { "*/*.go", }, wantOut: []string{ - "subDir1/subDir1_file.txt", - "subDir2/subDir2_file.txt", - "subDir2/subDir2_file.go", + filepath.Join("subDir1", "subDir1_file.txt"), + filepath.Join("subDir2", "subDir2_file.txt"), + filepath.Join("subDir2", "subDir2_file.go"), }, wantErr: nil, }, @@ -117,7 +118,7 @@ func TestGlobWindowsPaths(t *testing.T) { cleanupFn := createTestDir(t) defer cleanupFn() - got, err := GlobWindowsPaths(tt.patterns) + got, err := GlobPaths(tt.patterns) if tt.wantErr != nil { assert.EqualError(t, err, tt.wantErr.Error()) } else { From 8ace163608091804a42bd652b4b8bd7da3836f06 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Tue, 25 Feb 2025 10:49:28 +0500 Subject: [PATCH 05/25] Remove slicing --- pkg/cmd/gist/create/create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index 3eda8b0bd..defd57d95 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -88,7 +88,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co }, Aliases: []string{"new"}, RunE: func(c *cobra.Command, args []string) error { - opts.Filenames = args[:] + opts.Filenames = args if runF != nil { return runF(&opts) From 3d726c9865ae545ddc0802986c5f93187be36812 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Tue, 25 Feb 2025 17:58:51 +0500 Subject: [PATCH 06/25] Add tests for - and label args --- pkg/cmdutil/args_test.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/pkg/cmdutil/args_test.go b/pkg/cmdutil/args_test.go index c2602bbe9..e82dd4193 100644 --- a/pkg/cmdutil/args_test.go +++ b/pkg/cmdutil/args_test.go @@ -72,7 +72,19 @@ func TestGlobPaths(t *testing.T) { wantErr: nil, }, { - name: "When no files match, it returns an empty expansions array, it returns the unmatched patterns", + name: "When - is passed, return -", + patterns: []string{"-"}, + wantOut: []string{"-"}, + wantErr: nil, + }, + { + name: "When labels are passed, return labels", + patterns: []string{"file.txt#Text File", "README.md#README"}, + wantOut: []string{"file.txt#Text File", "README.md#README"}, + wantErr: nil, + }, + { + name: "When no files match, it returns an empty expansions array with error", patterns: []string{"foo"}, wantOut: []string{}, wantErr: errors.New("no matches found for `foo`"), From f4b65b785e9f0025b0e674cf78050a115924128d Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Wed, 26 Feb 2025 12:46:56 +0500 Subject: [PATCH 07/25] Add `exclude` callback function --- pkg/cmd/gist/create/create.go | 2 +- pkg/cmd/release/shared/upload.go | 4 +++- pkg/cmdutil/args.go | 5 ++--- pkg/cmdutil/args_test.go | 5 ++++- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index defd57d95..e212a08f3 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -105,7 +105,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } func createRun(opts *CreateOptions) error { - filenames, err := cmdutil.GlobPaths(opts.Filenames) + filenames, err := cmdutil.GlobPaths(opts.Filenames, nil) if err != nil { return err } diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 0b6468c3c..be30c010b 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -37,7 +37,9 @@ type AssetForUpload struct { } func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { - args, err = cmdutil.GlobPaths(args) + args, err = cmdutil.GlobPaths(args, func(pattern string) bool { + return strings.Contains(pattern, "#") + }) if err != nil { return nil, err } diff --git a/pkg/cmdutil/args.go b/pkg/cmdutil/args.go index 6bbe8c300..5f8b0d6e5 100644 --- a/pkg/cmdutil/args.go +++ b/pkg/cmdutil/args.go @@ -3,7 +3,6 @@ package cmdutil import ( "fmt" "path/filepath" - "strings" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -60,11 +59,11 @@ func NoArgsQuoteReminder(cmd *cobra.Command, args []string) error { return FlagErrorf("%s", errMsg) } -func GlobPaths(patterns []string) ([]string, error) { +func GlobPaths(patterns []string, exclude func(pattern string) bool) ([]string, error) { expansions := []string{} for _, pattern := range patterns { - if pattern == "-" || strings.Contains(pattern, "#") { + if pattern == "-" || (exclude != nil && exclude(pattern)) { expansions = append(expansions, pattern) } else { matches, err := filepath.Glob(pattern) diff --git a/pkg/cmdutil/args_test.go b/pkg/cmdutil/args_test.go index e82dd4193..bdb7e70ed 100644 --- a/pkg/cmdutil/args_test.go +++ b/pkg/cmdutil/args_test.go @@ -4,6 +4,7 @@ import ( "errors" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -130,7 +131,9 @@ func TestGlobPaths(t *testing.T) { cleanupFn := createTestDir(t) defer cleanupFn() - got, err := GlobPaths(tt.patterns) + got, err := GlobPaths(tt.patterns, func(pattern string) bool { + return strings.Contains(pattern, "#") + }) if tt.wantErr != nil { assert.EqualError(t, err, tt.wantErr.Error()) } else { From 0bb92c598017dd98d965672d2e5185878a538b22 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 3 Mar 2025 18:05:23 -0800 Subject: [PATCH 08/25] Separate partitioning from globbing in cmdutil/args package and consumers (#3) * Separate partitioning from globbing in cmdutil/args package and consumers In the previous commit, GlobPaths was overloaded, containing logic specific to command use-cases. This commit removes that functionality from GlobPaths and back into the commands that have the special use-cases. To do this, I've introduced a new Partition util in cmdutil/args.go that will separate a slice into two slices given a predicate. This functionality is leveraged by both the special use-cases described above to separate the command-specific syntax from the globable filepaths. * Add test to validate that the order of '-' in gh gist create args doesn't matter --- pkg/cmd/gist/create/create.go | 9 +++- pkg/cmd/gist/create/create_test.go | 26 +++++++++- pkg/cmd/release/shared/upload.go | 9 +++- pkg/cmdutil/args.go | 43 +++++++++++----- pkg/cmdutil/args_test.go | 80 ++++++++++++++++++++++++------ 5 files changed, 135 insertions(+), 32 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index 4e950b77b..5392b997e 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -105,11 +105,18 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } func createRun(opts *CreateOptions) error { - filenames, err := cmdutil.GlobPaths(opts.Filenames, nil) + + readFromStdInArg, filenames := cmdutil.Partition(opts.Filenames, func(f string) bool { + return f == "-" + }) + + filenames, err := cmdutil.GlobPaths(filenames) if err != nil { return err } + filenames = append(filenames, readFromStdInArg...) + if len(filenames) == 0 { filenames = []string{"-"} } diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index 237486eb3..44f0ba284 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -225,7 +225,7 @@ func Test_createRun(t *testing.T) { responseStatus: http.StatusOK, }, { - name: "multiple files", + name: "when both a file and the stdin '-' are provided, it matches on all the files passed in and stdin", opts: &CreateOptions{ Filenames: []string{fixtureFile, "-"}, }, @@ -248,6 +248,30 @@ func Test_createRun(t *testing.T) { }, responseStatus: http.StatusOK, }, + { + name: "when both a file and the stdin '-' are provided, but '-' is not the last argument, it matches on all the files provided and stdin", + opts: &CreateOptions{ + Filenames: []string{"-", fixtureFile}, + }, + stdin: "cool stdin content", + wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", + wantStderr: "- Creating gist with multiple files\n✓ Created secret gist fixture.txt\n", + wantErr: false, + wantParams: map[string]interface{}{ + "description": "", + "updated_at": "0001-01-01T00:00:00Z", + "public": false, + "files": map[string]interface{}{ + "fixture.txt": map[string]interface{}{ + "content": "{}", + }, + "gistfile1.txt": map[string]interface{}{ + "content": "cool stdin content", + }, + }, + }, + responseStatus: http.StatusOK, + }, { name: "file with empty content", opts: &CreateOptions{ diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index be30c010b..ab7533320 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -37,12 +37,17 @@ type AssetForUpload struct { } func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { - args, err = cmdutil.GlobPaths(args, func(pattern string) bool { - return strings.Contains(pattern, "#") + labeledArgs, unlabeledArgs := cmdutil.Partition(args, func(arg string) bool { + return strings.Contains(arg, "#") }) + + args, err = cmdutil.GlobPaths(unlabeledArgs) if err != nil { return nil, err } + + args = append(args, labeledArgs...) + for _, arg := range args { var label string fn := arg diff --git a/pkg/cmdutil/args.go b/pkg/cmdutil/args.go index 5f8b0d6e5..b0a22e9b5 100644 --- a/pkg/cmdutil/args.go +++ b/pkg/cmdutil/args.go @@ -59,22 +59,41 @@ func NoArgsQuoteReminder(cmd *cobra.Command, args []string) error { return FlagErrorf("%s", errMsg) } -func GlobPaths(patterns []string, exclude func(pattern string) bool) ([]string, error) { +// Partition takes a slice of any type T and separates it into two slices +// of the same type based on the provided predicate function. Any item +// that returns true for the predicate will be included in the first slice +// returned, and any item that returns false for the predicate will be +// included in the second slice returned. +func Partition[T any](slice []T, predicate func(T) bool) ([]T, []T) { + var matching, nonMatching []T + for _, item := range slice { + if predicate(item) { + matching = append(matching, item) + } else { + nonMatching = append(nonMatching, item) + } + } + return matching, nonMatching +} + +// GlobPaths expands a list of file patterns into a list of file paths. +// If no files match a pattern, that pattern will return an error. +// If no pattern is passed, this returns an empty list and no error. +// +// For information on supported glob patterns, see +// https://pkg.go.dev/path/filepath#Match +func GlobPaths(patterns []string) ([]string, error) { expansions := []string{} for _, pattern := range patterns { - if pattern == "-" || (exclude != nil && exclude(pattern)) { - expansions = append(expansions, pattern) - } else { - matches, err := filepath.Glob(pattern) - if err != nil { - return nil, fmt.Errorf("%s: %v", pattern, err) - } - if len(matches) == 0 { - return []string{}, fmt.Errorf("no matches found for `%s`", pattern) - } - expansions = append(expansions, matches...) + matches, err := filepath.Glob(pattern) + if err != nil { + return nil, fmt.Errorf("%s: %v", pattern, err) } + if len(matches) == 0 { + return []string{}, fmt.Errorf("no matches found for `%s`", pattern) + } + expansions = append(expansions, matches...) } return expansions, nil diff --git a/pkg/cmdutil/args_test.go b/pkg/cmdutil/args_test.go index bdb7e70ed..4e880dd27 100644 --- a/pkg/cmdutil/args_test.go +++ b/pkg/cmdutil/args_test.go @@ -4,7 +4,6 @@ import ( "errors" "os" "path/filepath" - "strings" "testing" "github.com/stretchr/testify/assert" @@ -58,6 +57,69 @@ func TestMinimumNs_with_error(t *testing.T) { } } +func TestPartition(t *testing.T) { + tests := []struct { + name string + slice []any + predicate func(any) bool + wantMatching []any + wantNonMatching []any + }{ + { + name: "When the slice is empty, it returns two empty slices", + slice: []any{}, + predicate: func(any) bool { + return true + }, + wantMatching: []any{}, + wantNonMatching: []any{}, + }, + { + name: "when the slice has one element that satisfies the predicate, it returns a slice with that element and an empty slice", + slice: []any{ + "foo", + }, + predicate: func(any) bool { + return true + }, + wantMatching: []any{"foo"}, + wantNonMatching: []any{}, + }, + { + name: "when the slice has one element that does not satisfy the predicate, it returns an empty slice and a slice with that element", + slice: []any{ + "foo", + }, + predicate: func(any) bool { + return false + }, + wantMatching: []any{}, + wantNonMatching: []any{"foo"}, + }, + { + name: "when the slice has multiple elements, it returns a slice with the elements that satisfy the predicate and a slice with the elements that do not satisfy the predicate", + slice: []any{ + "foo", + "bar", + "baz", + }, + predicate: func(s any) bool { + return s.(string) != "foo" + }, + wantMatching: []any{"bar", "baz"}, + wantNonMatching: []any{"foo"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotMatching, gotNonMatching := Partition(tt.slice, tt.predicate) + assert.ElementsMatch(t, tt.wantMatching, gotMatching) + assert.ElementsMatch(t, tt.wantNonMatching, gotNonMatching) + }) + } +} + func TestGlobPaths(t *testing.T) { tests := []struct { name string @@ -72,18 +134,6 @@ func TestGlobPaths(t *testing.T) { wantOut: []string{}, wantErr: nil, }, - { - name: "When - is passed, return -", - patterns: []string{"-"}, - wantOut: []string{"-"}, - wantErr: nil, - }, - { - name: "When labels are passed, return labels", - patterns: []string{"file.txt#Text File", "README.md#README"}, - wantOut: []string{"file.txt#Text File", "README.md#README"}, - wantErr: nil, - }, { name: "When no files match, it returns an empty expansions array with error", patterns: []string{"foo"}, @@ -131,9 +181,7 @@ func TestGlobPaths(t *testing.T) { cleanupFn := createTestDir(t) defer cleanupFn() - got, err := GlobPaths(tt.patterns, func(pattern string) bool { - return strings.Contains(pattern, "#") - }) + got, err := GlobPaths(tt.patterns) if tt.wantErr != nil { assert.EqualError(t, err, tt.wantErr.Error()) } else { From 601c3e448c14d4a1f649f7203ffd9941e6a3f9e0 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 5 Mar 2025 12:40:49 -0700 Subject: [PATCH 09/25] Fix(ci): base64 decode GPG passphrase --- .github/workflows/deployment.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deployment.yml b/.github/workflows/deployment.yml index ced49a9ac..60354a953 100644 --- a/.github/workflows/deployment.yml +++ b/.github/workflows/deployment.yml @@ -301,7 +301,7 @@ jobs: base64 -d <<<"$GPG_KEY" | gpg --import --no-tty --batch --yes echo "allow-preset-passphrase" > ~/.gnupg/gpg-agent.conf gpg-connect-agent RELOADAGENT /bye - /usr/lib/gnupg2/gpg-preset-passphrase --preset "$GPG_KEYGRIP" <<<"$GPG_PASSPHRASE" + base64 -d <<<"$GPG_PASSPHRASE" | /usr/lib/gnupg2/gpg-preset-passphrase --preset "$GPG_KEYGRIP" - name: Sign RPMs if: inputs.environment == 'production' run: | From db8b38a185ab420b1e9a254dbc72f0794ed9a5a7 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Mar 2025 11:45:22 +0100 Subject: [PATCH 10/25] Fix secret command panic when base repo from cwd --- pkg/cmd/secret/delete/delete.go | 6 ++--- pkg/cmd/secret/delete/delete_test.go | 34 ++++++++++++++++++++++------ pkg/cmd/secret/list/list.go | 6 ++--- pkg/cmd/secret/list/list_test.go | 34 ++++++++++++++++++++++------ pkg/cmd/secret/set/set.go | 6 ++--- pkg/cmd/secret/set/set_test.go | 34 ++++++++++++++++++++++------ 6 files changed, 90 insertions(+), 30 deletions(-) diff --git a/pkg/cmd/secret/delete/delete.go b/pkg/cmd/secret/delete/delete.go index 1cbfbca9d..b73b59848 100644 --- a/pkg/cmd/secret/delete/delete.go +++ b/pkg/cmd/secret/delete/delete.go @@ -49,9 +49,9 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co RunE: func(cmd *cobra.Command, args []string) error { // If the user specified a repo directly, then we're using the OverrideBaseRepoFunc set by EnableRepoOverride // So there's no reason to use the specialised BaseRepoFunc that requires remote disambiguation. - if cmd.Flags().Changed("repo") || os.Getenv("GH_REPO") != "" { - opts.BaseRepo = f.BaseRepo - } else { + opts.BaseRepo = f.BaseRepo + isRepoUserProvided := cmd.Flags().Changed("repo") || os.Getenv("GH_REPO") != "" + if !isRepoUserProvided { // 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) diff --git a/pkg/cmd/secret/delete/delete_test.go b/pkg/cmd/secret/delete/delete_test.go index 2f6cad431..48200b881 100644 --- a/pkg/cmd/secret/delete/delete_test.go +++ b/pkg/cmd/secret/delete/delete_test.go @@ -126,7 +126,7 @@ func TestNewCmdDelete(t *testing.T) { } func TestNewCmdDeleteBaseRepoFuncs(t *testing.T) { - remotes := ghContext.Remotes{ + multipleRemotes := ghContext.Remotes{ &ghContext.Remote{ Remote: &git.Remote{ Name: "origin", @@ -141,10 +141,20 @@ func TestNewCmdDeleteBaseRepoFuncs(t *testing.T) { }, } + singleRemote := ghContext.Remotes{ + &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "repo"), + }, + } + tests := []struct { name string args string env map[string]string + remotes ghContext.Remotes prompterStubs func(*prompter.MockPrompter) wantRepo ghrepo.Interface wantErr error @@ -152,6 +162,7 @@ func TestNewCmdDeleteBaseRepoFuncs(t *testing.T) { { name: "when there is a repo flag provided, the factory base repo func is used", args: "SECRET_NAME --repo owner/repo", + remotes: multipleRemotes, wantRepo: ghrepo.New("owner", "repo"), }, { @@ -160,18 +171,27 @@ func TestNewCmdDeleteBaseRepoFuncs(t *testing.T) { env: map[string]string{ "GH_REPO": "owner/repo", }, + remotes: multipleRemotes, wantRepo: ghrepo.New("owner", "repo"), }, { - name: "when there is no repo flag or GH_REPO env var provided, and no prompting, the base func requiring no ambiguity is used", - args: "SECRET_NAME", + name: "when there is no repo flag or GH_REPO env var provided, and no prompting, the base func requiring no ambiguity is used", + args: "SECRET_NAME", + remotes: multipleRemotes, wantErr: shared.AmbiguousBaseRepoError{ - Remotes: remotes, + Remotes: multipleRemotes, }, }, { - name: "when there is no repo flag or GH_REPO env var provided, and can prompt, the base func resolving ambiguity is used", - args: "SECRET_NAME", + name: "when there is no repo flag or GH_REPO env provided, and there is a single remote, the factory base repo func is used", + args: "SECRET_NAME", + remotes: singleRemote, + wantRepo: ghrepo.New("owner", "repo"), + }, + { + name: "when there is no repo flag or GH_REPO env var provided, and can prompt, the base func resolving ambiguity is used", + args: "SECRET_NAME", + remotes: multipleRemotes, prompterStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect( "Select a repo", @@ -204,7 +224,7 @@ func TestNewCmdDeleteBaseRepoFuncs(t *testing.T) { }, Prompter: pm, Remotes: func() (ghContext.Remotes, error) { - return remotes, nil + return tt.remotes, nil }, } diff --git a/pkg/cmd/secret/list/list.go b/pkg/cmd/secret/list/list.go index c976bd60e..06476a86d 100644 --- a/pkg/cmd/secret/list/list.go +++ b/pkg/cmd/secret/list/list.go @@ -70,9 +70,9 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman RunE: func(cmd *cobra.Command, args []string) error { // If the user specified a repo directly, then we're using the OverrideBaseRepoFunc set by EnableRepoOverride // So there's no reason to use the specialised BaseRepoFunc that requires remote disambiguation. - if cmd.Flags().Changed("repo") || os.Getenv("GH_REPO") != "" { - opts.BaseRepo = f.BaseRepo - } else { + opts.BaseRepo = f.BaseRepo + isRepoUserProvided := cmd.Flags().Changed("repo") || os.Getenv("GH_REPO") != "" + if !isRepoUserProvided { // 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) diff --git a/pkg/cmd/secret/list/list_test.go b/pkg/cmd/secret/list/list_test.go index f4ecde73e..5c4dd4874 100644 --- a/pkg/cmd/secret/list/list_test.go +++ b/pkg/cmd/secret/list/list_test.go @@ -106,7 +106,7 @@ func Test_NewCmdList(t *testing.T) { } func TestNewCmdListBaseRepoFuncs(t *testing.T) { - remotes := ghContext.Remotes{ + multipleRemotes := ghContext.Remotes{ &ghContext.Remote{ Remote: &git.Remote{ Name: "origin", @@ -121,10 +121,20 @@ func TestNewCmdListBaseRepoFuncs(t *testing.T) { }, } + singleRemote := ghContext.Remotes{ + &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "repo"), + }, + } + tests := []struct { name string args string env map[string]string + remotes ghContext.Remotes prompterStubs func(*prompter.MockPrompter) wantRepo ghrepo.Interface wantErr error @@ -132,6 +142,7 @@ func TestNewCmdListBaseRepoFuncs(t *testing.T) { { name: "when there is a repo flag provided, the factory base repo func is used", args: "--repo owner/repo", + remotes: multipleRemotes, wantRepo: ghrepo.New("owner", "repo"), }, { @@ -139,18 +150,27 @@ func TestNewCmdListBaseRepoFuncs(t *testing.T) { env: map[string]string{ "GH_REPO": "owner/repo", }, + remotes: multipleRemotes, wantRepo: ghrepo.New("owner", "repo"), }, { - name: "when there is no repo flag or GH_REPO env var provided, and no prompting, the base func requiring no ambiguity is used", - args: "", + name: "when there is no repo flag or GH_REPO env var provided, and no prompting, the base func requiring no ambiguity is used", + args: "", + remotes: multipleRemotes, wantErr: shared.AmbiguousBaseRepoError{ - Remotes: remotes, + Remotes: multipleRemotes, }, }, { - name: "when there is no repo flag or GH_REPO env var provided, and can prompt, the base func resolving ambiguity is used", - args: "", + name: "when there is no repo flag or GH_REPO env provided, and there is a single remote, the factory base repo func is used", + args: "", + remotes: singleRemote, + wantRepo: ghrepo.New("owner", "repo"), + }, + { + name: "when there is no repo flag or GH_REPO env var provided, and can prompt, the base func resolving ambiguity is used", + args: "", + remotes: multipleRemotes, prompterStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect( "Select a repo", @@ -183,7 +203,7 @@ func TestNewCmdListBaseRepoFuncs(t *testing.T) { }, Prompter: pm, Remotes: func() (ghContext.Remotes, error) { - return remotes, nil + return tt.remotes, nil }, } diff --git a/pkg/cmd/secret/set/set.go b/pkg/cmd/secret/set/set.go index 89f11f1cf..0a6581559 100644 --- a/pkg/cmd/secret/set/set.go +++ b/pkg/cmd/secret/set/set.go @@ -106,9 +106,9 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command RunE: func(cmd *cobra.Command, args []string) error { // If the user specified a repo directly, then we're using the OverrideBaseRepoFunc set by EnableRepoOverride // So there's no reason to use the specialised BaseRepoFunc that requires remote disambiguation. - if cmd.Flags().Changed("repo") || os.Getenv("GH_REPO") != "" { - opts.BaseRepo = f.BaseRepo - } else { + opts.BaseRepo = f.BaseRepo + isRepoUserProvided := cmd.Flags().Changed("repo") || os.Getenv("GH_REPO") != "" + if !isRepoUserProvided { // 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) diff --git a/pkg/cmd/secret/set/set_test.go b/pkg/cmd/secret/set/set_test.go index 22c7aaab8..0b305eda6 100644 --- a/pkg/cmd/secret/set/set_test.go +++ b/pkg/cmd/secret/set/set_test.go @@ -225,7 +225,7 @@ func TestNewCmdSet(t *testing.T) { } func TestNewCmdSetBaseRepoFuncs(t *testing.T) { - remotes := ghContext.Remotes{ + multipleRemotes := ghContext.Remotes{ &ghContext.Remote{ Remote: &git.Remote{ Name: "origin", @@ -240,10 +240,20 @@ func TestNewCmdSetBaseRepoFuncs(t *testing.T) { }, } + singleRemote := ghContext.Remotes{ + &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "repo"), + }, + } + tests := []struct { name string args string env map[string]string + remotes ghContext.Remotes prompterStubs func(*prompter.MockPrompter) wantRepo ghrepo.Interface wantErr error @@ -251,6 +261,7 @@ func TestNewCmdSetBaseRepoFuncs(t *testing.T) { { name: "when there is a repo flag provided, the factory base repo func is used", args: "SECRET_NAME --repo owner/repo", + remotes: multipleRemotes, wantRepo: ghrepo.New("owner", "repo"), }, { @@ -259,18 +270,27 @@ func TestNewCmdSetBaseRepoFuncs(t *testing.T) { env: map[string]string{ "GH_REPO": "owner/repo", }, + remotes: multipleRemotes, wantRepo: ghrepo.New("owner", "repo"), }, { - name: "when there is no repo flag or GH_REPO env var provided, and no prompting, the base func requiring no ambiguity is used", - args: "SECRET_NAME", + name: "when there is no repo flag or GH_REPO env var provided, and no prompting, the base func requiring no ambiguity is used", + args: "SECRET_NAME", + remotes: multipleRemotes, wantErr: shared.AmbiguousBaseRepoError{ - Remotes: remotes, + Remotes: multipleRemotes, }, }, { - name: "when there is no repo flag or GH_REPO env var provided, and can prompt, the base func resolving ambiguity is used", - args: "SECRET_NAME", + name: "when there is no repo flag or GH_REPO env provided, and there is a single remote, the factory base repo func is used", + args: "SECRET_NAME", + remotes: singleRemote, + wantRepo: ghrepo.New("owner", "repo"), + }, + { + name: "when there is no repo flag or GH_REPO env var provided, and can prompt, the base func resolving ambiguity is used", + args: "SECRET_NAME", + remotes: multipleRemotes, prompterStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect( "Select a repo", @@ -303,7 +323,7 @@ func TestNewCmdSetBaseRepoFuncs(t *testing.T) { }, Prompter: pm, Remotes: func() (ghContext.Remotes, error) { - return remotes, nil + return tt.remotes, nil }, } From 09b233746cdc009ebc49dccb4b0114376efc53b9 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Thu, 6 Mar 2025 11:12:56 -0800 Subject: [PATCH 11/25] Add cli-discuss-automation environment to triage.md Previously, we were getting the token from repository secrets. We have moved the token to its own environment secret in the cli-discuss-automation environment. It is in its own environment so that we don't inject our other secrets into this workflow as we don't need them here. --- .github/workflows/triage.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/triage.yml b/.github/workflows/triage.yml index 849beebad..3ed27a5f2 100644 --- a/.github/workflows/triage.yml +++ b/.github/workflows/triage.yml @@ -11,6 +11,7 @@ env: TARGET_REPO: github/cli jobs: issue: + environment: cli-discuss-automation runs-on: ubuntu-latest if: github.event_name == 'issues' && github.event.action == 'labeled' && github.event.label.name == 'discuss' steps: @@ -42,6 +43,7 @@ jobs: pull_request: runs-on: ubuntu-latest + environment: cli-discuss-automation if: github.event_name == 'pull_request_target' && github.event.action == 'labeled' && github.event.label.name == 'discuss' steps: - name: Create issue based on source pull request From 824acc86dd86a745b3014bd5353b844959f3591e Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Thu, 6 Mar 2025 11:20:39 -0800 Subject: [PATCH 12/25] Add environment to prauto and issueauto workflows --- .github/workflows/issueauto.yml | 1 + .github/workflows/prauto.yml | 1 + 2 files changed, 2 insertions(+) diff --git a/.github/workflows/issueauto.yml b/.github/workflows/issueauto.yml index 40c4b36e7..1322f12ef 100644 --- a/.github/workflows/issueauto.yml +++ b/.github/workflows/issueauto.yml @@ -10,6 +10,7 @@ permissions: jobs: issue-auto: runs-on: ubuntu-latest + environemnt: production steps: - name: label incoming issue env: diff --git a/.github/workflows/prauto.yml b/.github/workflows/prauto.yml index b7ed90183..f9d5ea9e6 100644 --- a/.github/workflows/prauto.yml +++ b/.github/workflows/prauto.yml @@ -11,6 +11,7 @@ permissions: jobs: pr-auto: runs-on: ubuntu-latest + environment: production steps: - name: lint pr env: From e0533f9f73473daababa8f8169f40c9eeb5a4e28 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 7 Mar 2025 12:02:20 -0800 Subject: [PATCH 13/25] Change issueauto and prauto actions to use the cli-automation env --- .github/workflows/issueauto.yml | 2 +- .github/workflows/prauto.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/issueauto.yml b/.github/workflows/issueauto.yml index 1322f12ef..cfdcff764 100644 --- a/.github/workflows/issueauto.yml +++ b/.github/workflows/issueauto.yml @@ -10,7 +10,7 @@ permissions: jobs: issue-auto: runs-on: ubuntu-latest - environemnt: production + environment: cli-automation steps: - name: label incoming issue env: diff --git a/.github/workflows/prauto.yml b/.github/workflows/prauto.yml index f9d5ea9e6..40dfee846 100644 --- a/.github/workflows/prauto.yml +++ b/.github/workflows/prauto.yml @@ -11,7 +11,7 @@ permissions: jobs: pr-auto: runs-on: ubuntu-latest - environment: production + environment: cli-automation steps: - name: lint pr env: From b01288617a6e2f6eae459f49a0a8343b59b4e269 Mon Sep 17 00:00:00 2001 From: Kazuma Watanabe Date: Mon, 10 Mar 2025 03:22:09 +0000 Subject: [PATCH 14/25] Make missing workflow regexp aware of GitHub App Follow up of https://github.com/cli/cli/pull/7612 The `missingWorkflowScopeRE` is defined to capture the error message when the `GH_TOKEN` does not have `workflow` scope in `gh repo sync `, but this is only intended for error messages for OAuth Apps and does not work with GitHub Apps. In GitHub App, you will get the following error: ``` { "message": "refusing to allow a GitHub App to create or update workflow `.github/workflows/teamcity-pr-checks.yml` without `workflows` permission", "documentation_url": "https://docs.github.com/rest/branches/branches#sync-a-fork-branch-with-the-upstream-repository", "status": "422" } ``` As you can see above, the existing regexp does not match the "`workflows` permission". This change modifies the regexp to return the user-friendly error message when the `workflow` permission is missing, even in the case of a GitHub App. --- pkg/cmd/repo/sync/http.go | 4 ++-- pkg/cmd/repo/sync/sync_test.go | 40 ++++++++++++++++++++++++++++++++-- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/repo/sync/http.go b/pkg/cmd/repo/sync/http.go index 3e193daeb..27e9a6351 100644 --- a/pkg/cmd/repo/sync/http.go +++ b/pkg/cmd/repo/sync/http.go @@ -32,8 +32,8 @@ func latestCommit(client *api.Client, repo ghrepo.Interface, branch string) (com type upstreamMergeErr struct{ error } -var missingWorkflowScopeRE = regexp.MustCompile("refusing to allow.*without `workflow` scope") -var missingWorkflowScopeErr = errors.New("Upstream commits contain workflow changes, which require the `workflow` scope to merge. To request it, run: gh auth refresh -s workflow") +var missingWorkflowScopeRE = regexp.MustCompile("refusing to allow.*without `workflow(s)?` (scope|permission)") +var missingWorkflowScopeErr = errors.New("Upstream commits contain workflow changes, which require the `workflow` scope or permission to merge. To request it, run: gh auth refresh -s workflow") func triggerUpstreamMerge(client *api.Client, repo ghrepo.Interface, branch string) (string, error) { var payload bytes.Buffer diff --git a/pkg/cmd/repo/sync/sync_test.go b/pkg/cmd/repo/sync/sync_test.go index 227e2a6a2..60e6ae392 100644 --- a/pkg/cmd/repo/sync/sync_test.go +++ b/pkg/cmd/repo/sync/sync_test.go @@ -470,7 +470,7 @@ func Test_SyncRun(t *testing.T) { errMsg: "trunk branch does not exist on OWNER/REPO-FORK repository", }, { - name: "sync remote fork with missing workflow scope on token", + name: "sync remote fork with missing workflow scope on OAuth App token", opts: &SyncOptions{ DestArg: "FORKOWNER/REPO-FORK", }, @@ -485,7 +485,43 @@ func Test_SyncRun(t *testing.T) { }{Message: "refusing to allow an OAuth App to create or update workflow `.github/workflows/unimportant.yml` without `workflow` scope"})) }, wantErr: true, - errMsg: "Upstream commits contain workflow changes, which require the `workflow` scope to merge. To request it, run: gh auth refresh -s workflow", + errMsg: "Upstream commits contain workflow changes, which require the `workflow` scope or permission to merge. To request it, run: gh auth refresh -s workflow", + }, + { + name: "sync remote fork with missing workflow permission on GitHub App token", + opts: &SyncOptions{ + DestArg: "FORKOWNER/REPO-FORK", + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`)) + reg.Register( + httpmock.REST("POST", "repos/FORKOWNER/REPO-FORK/merge-upstream"), + httpmock.StatusJSONResponse(422, struct { + Message string `json:"message"` + }{Message: "refusing to allow a GitHub App to create or update workflow `.github/workflows/unimportant.yml` without `workflows` permission"})) + }, + wantErr: true, + errMsg: "Upstream commits contain workflow changes, which require the `workflow` scope or permission to merge. To request it, run: gh auth refresh -s workflow", + }, + { + name: "sync remote fork with missing workflow scope on personal access token", + opts: &SyncOptions{ + DestArg: "FORKOWNER/REPO-FORK", + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`)) + reg.Register( + httpmock.REST("POST", "repos/FORKOWNER/REPO-FORK/merge-upstream"), + httpmock.StatusJSONResponse(422, struct { + Message string `json:"message"` + }{Message: "refusing to allow a Personal Access Token to create or update workflow `.github/workflows/unimportant.yml` without `workflow` scope"})) + }, + wantErr: true, + errMsg: "Upstream commits contain workflow changes, which require the `workflow` scope or permission to merge. To request it, run: gh auth refresh -s workflow", }, } for _, tt := range tests { From e09b9a5fae2c543ee196f5255f9b8ceb172bb2e3 Mon Sep 17 00:00:00 2001 From: kevincatty Date: Wed, 12 Mar 2025 16:13:39 +0800 Subject: [PATCH 15/25] chore: remove redundant word in comment Signed-off-by: kevincatty --- pkg/cmd/pr/update-branch/update_branch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/update-branch/update_branch.go b/pkg/cmd/pr/update-branch/update_branch.go index 023fdae50..ca3b2c910 100644 --- a/pkg/cmd/pr/update-branch/update_branch.go +++ b/pkg/cmd/pr/update-branch/update_branch.go @@ -43,7 +43,7 @@ func NewCmdUpdateBranch(f *cmdutil.Factory, runF func(*UpdateBranchOptions) erro Without an argument, the pull request that belongs to the current branch is selected. The default behavior is to update with a merge commit (i.e., merging the base branch - into the the PR's branch). To reconcile the changes with rebasing on top of the base + into the PR's branch). To reconcile the changes with rebasing on top of the base branch, the %[1]s--rebase%[1]s option should be provided. `, "`"), Example: heredoc.Doc(` From 7901f2364c50325aa1304c9442fb894eed6106b0 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 13 Mar 2025 01:52:40 +0000 Subject: [PATCH 16/25] Bump golang.org/x/net from 0.34.0 to 0.36.0 Bumps [golang.org/x/net](https://github.com/golang/net) from 0.34.0 to 0.36.0. - [Commits](https://github.com/golang/net/compare/v0.34.0...v0.36.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: indirect ... Signed-off-by: dependabot[bot] --- go.mod | 12 ++++++------ go.sum | 24 ++++++++++++------------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/go.mod b/go.mod index 9d1afbc38..433156789 100644 --- a/go.mod +++ b/go.mod @@ -46,10 +46,10 @@ require ( github.com/spf13/pflag v1.0.6 github.com/stretchr/testify v1.10.0 github.com/zalando/go-keyring v0.2.5 - golang.org/x/crypto v0.32.0 - golang.org/x/sync v0.10.0 - golang.org/x/term v0.28.0 - golang.org/x/text v0.21.0 + golang.org/x/crypto v0.35.0 + golang.org/x/sync v0.11.0 + golang.org/x/term v0.29.0 + golang.org/x/text v0.22.0 google.golang.org/grpc v1.69.4 google.golang.org/protobuf v1.36.5 gopkg.in/h2non/gock.v1 v1.1.2 @@ -158,8 +158,8 @@ require ( go.uber.org/zap v1.27.0 // indirect golang.org/x/exp v0.0.0-20240325151524-a685a6edb6d8 // indirect golang.org/x/mod v0.22.0 // indirect - golang.org/x/net v0.34.0 // indirect - golang.org/x/sys v0.29.0 // indirect + golang.org/x/net v0.36.0 // indirect + golang.org/x/sys v0.30.0 // indirect golang.org/x/tools v0.29.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20241209162323-e6fa225c2576 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20250102185135-69823020774d // indirect diff --git a/go.sum b/go.sum index 2c14dc4c3..b5db03d9c 100644 --- a/go.sum +++ b/go.sum @@ -502,8 +502,8 @@ go.uber.org/zap v1.27.0 h1:aJMhYGrd5QSmlpLMr2MftRKl7t8J8PTZPA732ud/XR8= go.uber.org/zap v1.27.0/go.mod h1:GB2qFLM7cTU87MWRP2mPIjqfIDnGu+VIO4V/SdhGo2E= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= -golang.org/x/crypto v0.32.0 h1:euUpcYgM8WcP71gNpTqQCn6rC2t6ULUPiOzfWaXVVfc= -golang.org/x/crypto v0.32.0/go.mod h1:ZnnJkOaASj8g0AjIduWNlq2NRxL0PlBrbKVyZ6V/Ugc= +golang.org/x/crypto v0.35.0 h1:b15kiHdrGCHrP6LvwaQ3c03kgNhhiMgvlhxHQhmg2Xs= +golang.org/x/crypto v0.35.0/go.mod h1:dy7dXNW32cAb/6/PRuTNsix8T+vJAqvuIy5Bli/x0YQ= golang.org/x/exp v0.0.0-20240325151524-a685a6edb6d8 h1:aAcj0Da7eBAtrTp03QXWvm88pSyOt+UgdZw2BFZ+lEw= golang.org/x/exp v0.0.0-20240325151524-a685a6edb6d8/go.mod h1:CQ1k9gNrJ50XIzaKCRR2hssIjF07kZFEiieALBM/ARQ= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= @@ -512,14 +512,14 @@ golang.org/x/mod v0.22.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c= -golang.org/x/net v0.34.0 h1:Mb7Mrk043xzHgnRM88suvJFwzVrRfHEHJEl5/71CKw0= -golang.org/x/net v0.34.0/go.mod h1:di0qlW3YNM5oh6GqDGQr92MyTozJPmybPK4Ev/Gm31k= +golang.org/x/net v0.36.0 h1:vWF2fRbw4qslQsQzgFqZff+BItCvGFQqKzKIzx1rmoA= +golang.org/x/net v0.36.0/go.mod h1:bFmbeoIPfrw4sMHNhb4J9f6+tPziuGjq7Jk/38fxi1I= golang.org/x/oauth2 v0.25.0 h1:CY4y7XT9v0cRI9oupztF8AgiIu99L/ksR/Xp/6jrZ70= golang.org/x/oauth2 v0.25.0/go.mod h1:XYTD2NtWslqkgxebSiOHnXEap4TF09sJSc7H1sXbhtI= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.10.0 h1:3NQrjDixjgGwUOCaF8w2+VYHv0Ve/vGYSbdkTa98gmQ= -golang.org/x/sync v0.10.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/sync v0.11.0 h1:GGz8+XQP4FvTTrjZPzNKTMFtSXH80RAzG+5ghFPgK9w= +golang.org/x/sync v0.11.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190222072716-a9d3bda3a223/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= @@ -529,19 +529,19 @@ golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.29.0 h1:TPYlXGxvx1MGTn2GiZDhnjPA9wZzZeGKHHmKhHYvgaU= -golang.org/x/sys v0.29.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/sys v0.30.0 h1:QjkSwP/36a20jFYWkSue1YwXzLmsV5Gfq7Eiy72C1uc= +golang.org/x/sys v0.30.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= -golang.org/x/term v0.28.0 h1:/Ts8HFuMR2E6IP/jlo7QVLZHggjKQbhu/7H0LJFr3Gg= -golang.org/x/term v0.28.0/go.mod h1:Sw/lC2IAUZ92udQNf3WodGtn4k/XoLyZoh8v/8uiwek= +golang.org/x/term v0.29.0 h1:L6pJp37ocefwRRtYPKSWOWzOtWSxVajvz2ldH/xi3iU= +golang.org/x/term v0.29.0/go.mod h1:6bl4lRlvVuDgSf3179VpIxBF0o10JUpXWOnI7nErv7s= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= golang.org/x/text v0.4.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= golang.org/x/text v0.5.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= -golang.org/x/text v0.21.0 h1:zyQAAkrwaneQ066sspRyJaG9VNi/YJ1NfzcGB3hZ/qo= -golang.org/x/text v0.21.0/go.mod h1:4IBbMaMmOPCJ8SecivzSH54+73PCFmPWxNTLm+vZkEQ= +golang.org/x/text v0.22.0 h1:bofq7m3/HAFvbF51jz3Q9wLg3jkvSPuiZu/pD1XwgtM= +golang.org/x/text v0.22.0/go.mod h1:YRoo4H8PVmsu+E3Ou7cqLVH8oXWIHVoX0jqUWALQhfY= golang.org/x/time v0.9.0 h1:EsRrnYcQiGH+5FfbgvV4AP7qEZstoyrHB0DzarOQ4ZY= golang.org/x/time v0.9.0/go.mod h1:3BpzKBy/shNhVucY/MWOyx10tF3SFh9QdLuxbVysPQM= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= From f3e4976da39badaf2f07cc86f7041c88841b5b98 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Fri, 14 Mar 2025 11:51:09 +0500 Subject: [PATCH 17/25] `./script/sign` cleanup --- .goreleaser.yml | 2 +- docs/release-process-deep-dive.md | 3 --- script/sign | 33 +++---------------------------- 3 files changed, 4 insertions(+), 34 deletions(-) diff --git a/.goreleaser.yml b/.goreleaser.yml index a7b293d6e..9584f7403 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -40,7 +40,7 @@ builds: hooks: post: - cmd: >- - {{ if eq .Runtime.Goos "windows" }}pwsh .\script\sign.ps1{{ else }}./script/sign{{ end }} '{{ .Path }}' + {{ if eq .Runtime.Goos "windows" }}pwsh .\script\sign.ps1{{ end }} '{{ .Path }}' output: true binary: bin/gh main: ./cmd/gh diff --git a/docs/release-process-deep-dive.md b/docs/release-process-deep-dive.md index ed9362d38..31f44f6ef 100644 --- a/docs/release-process-deep-dive.md +++ b/docs/release-process-deep-dive.md @@ -428,9 +428,6 @@ Breaking this command down: * `/dlib` points to the previously extracted DLL * `/dmdf` points to the previously created metadata file -> [!WARNING] -> The [`GoReleaser` signing hook](https://github.com/cli/cli/blob/756f4ec04abdc9fdbab3fef35b182c546ef1dd17/.goreleaser.yml#L43) can currently call `./script/sign` on a non-windows machine, but this is an artifact from pre-HSM that should be removed. - ## [release](https://github.com/cli/cli/blob/756f4ec04abdc9fdbab3fef35b182c546ef1dd17/.github/workflows/deployment.yml#L250-L395)
diff --git a/script/sign b/script/sign index 1630a06b5..0c2a4602f 100755 --- a/script/sign +++ b/script/sign @@ -1,34 +1,10 @@ #!/bin/bash # usage: script/sign # -# Signs macOS binaries using codesign, notarizes macOS zip archives using notarytool, and signs -# Windows EXE and MSI files using osslsigncode. +# Signs macOS binaries using codesign, notarizes macOS zip archives using notarytool # set -e -sign_windows() { - if [ -z "$CERT_FILE" ]; then - echo "skipping Windows code-signing; CERT_FILE not set" >&2 - return 0 - fi - - if [ ! -f "$CERT_FILE" ]; then - echo "error Windows code-signing; file '$CERT_FILE' not found" >&2 - return 1 - fi - - if [ -z "$CERT_PASSWORD" ]; then - echo "error Windows code-signing; no value for CERT_PASSWORD" >&2 - return 1 - fi - - osslsigncode sign -n "GitHub CLI" -t http://timestamp.digicert.com \ - -pkcs12 "$CERT_FILE" -readpass <(printf "%s" "$CERT_PASSWORD") -h sha256 \ - -in "$1" -out "$1"~ - - mv "$1"~ "$1" -} - sign_macos() { if [ -z "$APPLE_DEVELOPER_ID" ]; then echo "skipping macOS code-signing; APPLE_DEVELOPER_ID not set" >&2 @@ -51,10 +27,7 @@ platform="$(uname -s)" for input_file; do case "$input_file" in - *.exe | *.msi ) - sign_windows "$input_file" - ;; - * ) + *) if [ "$platform" = "Darwin" ]; then sign_macos "$input_file" else @@ -62,4 +35,4 @@ for input_file; do fi ;; esac -done \ No newline at end of file +done From 342e3cd70c1896c71c4196d7f6fc225a27314c12 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Fri, 14 Mar 2025 12:17:24 +0500 Subject: [PATCH 18/25] More cleanup --- .goreleaser.yml | 3 +-- script/sign | 18 +++++++----------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/.goreleaser.yml b/.goreleaser.yml index 9584f7403..6ef1ecc8b 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -39,8 +39,7 @@ builds: goarch: [386, amd64, arm64] hooks: post: - - cmd: >- - {{ if eq .Runtime.Goos "windows" }}pwsh .\script\sign.ps1{{ end }} '{{ .Path }}' + - cmd: pwsh .\script\sign.ps1 '{{ .Path }}' output: true binary: bin/gh main: ./cmd/gh diff --git a/script/sign b/script/sign index 0c2a4602f..f07a7d2d4 100755 --- a/script/sign +++ b/script/sign @@ -6,7 +6,7 @@ set -e sign_macos() { - if [ -z "$APPLE_DEVELOPER_ID" ]; then + if [[ -z "$APPLE_DEVELOPER_ID" ]]; then echo "skipping macOS code-signing; APPLE_DEVELOPER_ID not set" >&2 return 0 fi @@ -18,21 +18,17 @@ sign_macos() { fi } -if [ $# -eq 0 ]; then +if [[ $# -eq 0 ]]; then echo "usage: script/sign " >&2 exit 1 fi platform="$(uname -s)" +if [[ $platform != "Darwin" ]]; then + echo "error: must run on macOS; skipping codesigning/notarization" >&2 + exit 1 +fi for input_file; do - case "$input_file" in - *) - if [ "$platform" = "Darwin" ]; then - sign_macos "$input_file" - else - printf "warning: don't know how to sign %s on %s\n" "$1", "$platform" >&2 - fi - ;; - esac + sign_macos "$input_file" done From 018b6d6d072994d96be2962e943b41fdb66bbaea Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 17 Mar 2025 23:18:36 -0400 Subject: [PATCH 19/25] Initial pass fixing gh issue and gh pr comment There is still a bit of work to get the gh pr comment tests in order, however this goes a way towards fixing the issue along with acceptance tests. Also, it turns out some of the issue acceptance tests were really running `pr` tests. --- acceptance/acceptance_test.go | 2 +- ...ssue-comment-edit-last-with-comments.txtar | 31 +++++ ...t-edit-last-without-comments-creates.txtar | 23 ++++ ...nt-edit-last-without-comments-errors.txtar | 20 ++++ ...-comment.txtar => issue-comment-new.txtar} | 11 +- .../pr-comment-edit-last-with-comments.txtar | 40 +++++++ ...t-edit-last-without-comments-creates.txtar | 32 +++++ ...nt-edit-last-without-comments-errors.txtar | 29 +++++ ...{pr-comment.txtar => pr-comment-new.txtar} | 11 +- pkg/cmd/issue/comment/comment_test.go | 109 +++++++++++------- pkg/cmd/pr/shared/commentable.go | 47 +++++--- 11 files changed, 285 insertions(+), 70 deletions(-) create mode 100644 acceptance/testdata/issue/issue-comment-edit-last-with-comments.txtar create mode 100644 acceptance/testdata/issue/issue-comment-edit-last-without-comments-creates.txtar create mode 100644 acceptance/testdata/issue/issue-comment-edit-last-without-comments-errors.txtar rename acceptance/testdata/issue/{issue-comment.txtar => issue-comment-new.txtar} (64%) create mode 100644 acceptance/testdata/pr/pr-comment-edit-last-with-comments.txtar create mode 100644 acceptance/testdata/pr/pr-comment-edit-last-without-comments-creates.txtar create mode 100644 acceptance/testdata/pr/pr-comment-edit-last-without-comments-errors.txtar rename acceptance/testdata/pr/{pr-comment.txtar => pr-comment-new.txtar} (72%) diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index b22929ed5..b8eff8389 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -69,7 +69,7 @@ func TestIssues(t *testing.T) { t.Fatal(err) } - testscript.Run(t, testScriptParamsFor(tsEnv, "pr")) + testscript.Run(t, testScriptParamsFor(tsEnv, "issue")) } func TestLabels(t *testing.T) { diff --git a/acceptance/testdata/issue/issue-comment-edit-last-with-comments.txtar b/acceptance/testdata/issue/issue-comment-edit-last-with-comments.txtar new file mode 100644 index 000000000..d63c2e17a --- /dev/null +++ b/acceptance/testdata/issue/issue-comment-edit-last-with-comments.txtar @@ -0,0 +1,31 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Create a repository with a file so it has a default branch +exec gh repo create $ORG/$REPO --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes $ORG/$REPO + +# Clone the repo +exec gh repo clone $ORG/$REPO + +# Create an issue in the repo +cd $REPO +exec gh issue create --title 'Feature Request' --body 'Feature Body' +stdout2env ISSUE_URL + +# Comment on the issue +exec gh issue comment $ISSUE_URL --body 'Looks like a great feature!' + +# View the issue +exec gh issue view $ISSUE_URL --comments +stdout 'Looks like a great feature!' + +# Edit the last comment +exec gh issue comment $ISSUE_URL --edit-last --body 'Looks like an amazing feature!' + +# View the issue +exec gh issue view $ISSUE_URL --comments +! stdout 'Looks like a great feature!' +stdout 'Looks like an amazing feature!' diff --git a/acceptance/testdata/issue/issue-comment-edit-last-without-comments-creates.txtar b/acceptance/testdata/issue/issue-comment-edit-last-without-comments-creates.txtar new file mode 100644 index 000000000..6dc09d4db --- /dev/null +++ b/acceptance/testdata/issue/issue-comment-edit-last-without-comments-creates.txtar @@ -0,0 +1,23 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Create a repository with a file so it has a default branch +exec gh repo create $ORG/$REPO --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes $ORG/$REPO + +# Clone the repo +exec gh repo clone $ORG/$REPO + +# Create an issue in the repo +cd $REPO +exec gh issue create --title 'Feature Request' --body 'Feature Body' +stdout2env ISSUE_URL + +# Comment on the issue +exec gh issue comment $ISSUE_URL --edit-last --create-if-none --body 'Looks like a great feature!' + +# View the issue +exec gh issue view $ISSUE_URL --comments +stdout 'Looks like a great feature!' diff --git a/acceptance/testdata/issue/issue-comment-edit-last-without-comments-errors.txtar b/acceptance/testdata/issue/issue-comment-edit-last-without-comments-errors.txtar new file mode 100644 index 000000000..050ff92d5 --- /dev/null +++ b/acceptance/testdata/issue/issue-comment-edit-last-without-comments-errors.txtar @@ -0,0 +1,20 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Create a repository with a file so it has a default branch +exec gh repo create $ORG/$REPO --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes $ORG/$REPO + +# Clone the repo +exec gh repo clone $ORG/$REPO + +# Create an issue in the repo +cd $REPO +exec gh issue create --title 'Feature Request' --body 'Feature Body' +stdout2env ISSUE_URL + +# Comment on the issue +! exec gh issue comment $ISSUE_URL --edit-last --body 'Looks like a great feature!' +stderr 'no comments found for current user' diff --git a/acceptance/testdata/issue/issue-comment.txtar b/acceptance/testdata/issue/issue-comment-new.txtar similarity index 64% rename from acceptance/testdata/issue/issue-comment.txtar rename to acceptance/testdata/issue/issue-comment-new.txtar index f47bf619c..2ce318629 100644 --- a/acceptance/testdata/issue/issue-comment.txtar +++ b/acceptance/testdata/issue/issue-comment-new.txtar @@ -1,14 +1,17 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + # Create a repository with a file so it has a default branch -exec gh repo create $ORG/$SCRIPT_NAME-$RANDOM_STRING --add-readme --private +exec gh repo create $ORG/$REPO --add-readme --private # Defer repo cleanup -defer gh repo delete --yes $ORG/$SCRIPT_NAME-$RANDOM_STRING +defer gh repo delete --yes $ORG/$REPO # Clone the repo -exec gh repo clone $ORG/$SCRIPT_NAME-$RANDOM_STRING +exec gh repo clone $ORG/$REPO # Create an issue in the repo -cd $SCRIPT_NAME-$RANDOM_STRING +cd $REPO exec gh issue create --title 'Feature Request' --body 'Feature Body' stdout2env ISSUE_URL diff --git a/acceptance/testdata/pr/pr-comment-edit-last-with-comments.txtar b/acceptance/testdata/pr/pr-comment-edit-last-with-comments.txtar new file mode 100644 index 000000000..5b371978c --- /dev/null +++ b/acceptance/testdata/pr/pr-comment-edit-last-with-comments.txtar @@ -0,0 +1,40 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository with a file so it has a default branch +exec gh repo create $ORG/$REPO --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes $ORG/$REPO + +# Clone the repo +exec gh repo clone $ORG/$REPO + +# Prepare a branch to PR +cd $REPO +exec git checkout -b feature-branch +exec git commit --allow-empty -m 'Empty Commit' +exec git push -u origin feature-branch + + +# Create the PR +exec gh pr create --title 'Feature Title' --body 'Feature Body' +stdout2env PR_URL + +# Comment on the PR +exec gh pr comment $PR_URL --body 'Looks like a great feature!' + +# View the PR +exec gh pr view $PR_URL --comments +stdout 'Looks like a great feature!' + +# Edit the last comment +exec gh pr comment $PR_URL --edit-last --body 'Looks like an amazing feature!' + +# View the PR +exec gh pr view $PR_URL --comments +! stdout 'Looks like a great feature!' +stdout 'Looks like an amazing feature!' diff --git a/acceptance/testdata/pr/pr-comment-edit-last-without-comments-creates.txtar b/acceptance/testdata/pr/pr-comment-edit-last-without-comments-creates.txtar new file mode 100644 index 000000000..4f2039f9d --- /dev/null +++ b/acceptance/testdata/pr/pr-comment-edit-last-without-comments-creates.txtar @@ -0,0 +1,32 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository with a file so it has a default branch +exec gh repo create $ORG/$REPO --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes $ORG/$REPO + +# Clone the repo +exec gh repo clone $ORG/$REPO + +# Prepare a branch to PR +cd $REPO +exec git checkout -b feature-branch +exec git commit --allow-empty -m 'Empty Commit' +exec git push -u origin feature-branch + + +# Create the PR +exec gh pr create --title 'Feature Title' --body 'Feature Body' +stdout2env PR_URL + +# Comment on the PR +exec gh pr comment $PR_URL --edit-last --create-if-none --body 'Looks like a great feature!' + +# View the PR +exec gh pr view $PR_URL --comments +stdout 'Looks like a great feature!' diff --git a/acceptance/testdata/pr/pr-comment-edit-last-without-comments-errors.txtar b/acceptance/testdata/pr/pr-comment-edit-last-without-comments-errors.txtar new file mode 100644 index 000000000..8f28db5cd --- /dev/null +++ b/acceptance/testdata/pr/pr-comment-edit-last-without-comments-errors.txtar @@ -0,0 +1,29 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository with a file so it has a default branch +exec gh repo create $ORG/$REPO --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes $ORG/$REPO + +# Clone the repo +exec gh repo clone $ORG/$REPO + +# Prepare a branch to PR +cd $REPO +exec git checkout -b feature-branch +exec git commit --allow-empty -m 'Empty Commit' +exec git push -u origin feature-branch + + +# Create the PR +exec gh pr create --title 'Feature Title' --body 'Feature Body' +stdout2env PR_URL + +# Comment on the PR +! exec gh pr comment $PR_URL --edit-last --body 'Looks like a great feature!' +stderr 'no comments found for current user' diff --git a/acceptance/testdata/pr/pr-comment.txtar b/acceptance/testdata/pr/pr-comment-new.txtar similarity index 72% rename from acceptance/testdata/pr/pr-comment.txtar rename to acceptance/testdata/pr/pr-comment-new.txtar index 2c4d488b5..9856c0430 100644 --- a/acceptance/testdata/pr/pr-comment.txtar +++ b/acceptance/testdata/pr/pr-comment-new.txtar @@ -1,17 +1,20 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + # Use gh as a credential helper exec gh auth setup-git # Create a repository with a file so it has a default branch -exec gh repo create $ORG/$SCRIPT_NAME-$RANDOM_STRING --add-readme --private +exec gh repo create $ORG/$REPO --add-readme --private # Defer repo cleanup -defer gh repo delete --yes $ORG/$SCRIPT_NAME-$RANDOM_STRING +defer gh repo delete --yes $ORG/$REPO # Clone the repo -exec gh repo clone $ORG/$SCRIPT_NAME-$RANDOM_STRING +exec gh repo clone $ORG/$REPO # Prepare a branch to PR -cd $SCRIPT_NAME-$RANDOM_STRING +cd $REPO exec git checkout -b feature-branch exec git commit --allow-empty -m 'Empty Commit' exec git push -u origin feature-branch diff --git a/pkg/cmd/issue/comment/comment_test.go b/pkg/cmd/issue/comment/comment_test.go index 461ae1065..c7feaf698 100644 --- a/pkg/cmd/issue/comment/comment_test.go +++ b/pkg/cmd/issue/comment/comment_test.go @@ -223,9 +223,10 @@ func Test_commentRun(t *testing.T) { httpStubs func(*testing.T, *httpmock.Registry) stdout string stderr string + wantsErr bool }{ { - name: "interactive editor", + name: "creating new issue comment with interactive editor succeeds", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -234,13 +235,29 @@ func Test_commentRun(t *testing.T) { InteractiveEditSurvey: func(string) (string, error) { return "comment body", nil }, ConfirmSubmitSurvey: func() (bool, error) { return true, nil }, }, + emptyComments: true, httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockCommentCreate(t, reg) }, stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", }, { - name: "interactive editor with edit last", + name: "updating last issue comment with interactive editor fails if there are no comments and decline prompt to create", + input: &shared.CommentableOptions{ + Interactive: true, + InputType: 0, + Body: "", + EditLast: true, + + InteractiveEditSurvey: func(string) (string, error) { return "comment body", nil }, + ConfirmSubmitSurvey: func() (bool, error) { return true, nil }, + ConfirmCreateIfNoneSurvey: func() (bool, error) { return false, nil }, + }, + emptyComments: true, + wantsErr: true, + }, + { + name: "updating last issue comment with interactive editor succeeds if there are comments", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -253,10 +270,11 @@ func Test_commentRun(t *testing.T) { httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockCommentUpdate(t, reg) }, - stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-111\n", + emptyComments: false, + stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-111\n", }, { - name: "interactive editor with edit last and create if none", + name: "updating last issue comment with interactive editor creates new comment if there are no comments but --create-if-none", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -269,12 +287,14 @@ func Test_commentRun(t *testing.T) { ConfirmSubmitSurvey: func() (bool, error) { return true, nil }, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockCommentUpdate(t, reg) + mockCommentCreate(t, reg) }, - stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-111\n", + emptyComments: true, + stderr: "No comments found. Creating a new comment.\n", + stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", }, { - name: "non-interactive web", + name: "creating new issue comment with non-interactive web opens issue in browser focusing on new comment", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeWeb, @@ -282,39 +302,11 @@ func Test_commentRun(t *testing.T) { OpenInBrowser: func(string) error { return nil }, }, - stderr: "Opening https://github.com/OWNER/REPO/issues/123 in your browser.\n", - }, - { - name: "non-interactive web with edit last", - input: &shared.CommentableOptions{ - Interactive: false, - InputType: shared.InputTypeWeb, - Body: "", - EditLast: true, - - OpenInBrowser: func(string) error { return nil }, - }, - stderr: "Opening https://github.com/OWNER/REPO/issues/123 in your browser.\n", - }, - { - name: "non-interactive web with edit last and create if none for empty comments", - input: &shared.CommentableOptions{ - Interactive: false, - InputType: shared.InputTypeWeb, - Body: "", - EditLast: true, - CreateIfNone: true, - - OpenInBrowser: func(u string) error { - assert.Contains(t, u, "#issuecomment-new") - return nil - }, - }, emptyComments: true, stderr: "Opening https://github.com/OWNER/REPO/issues/123 in your browser.\n", }, { - name: "non-interactive web with edit last and create if none", + name: "updating last issue comment with non-interactive web opens issue in browser focusing on the last comment", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeWeb, @@ -330,7 +322,19 @@ func Test_commentRun(t *testing.T) { stderr: "Opening https://github.com/OWNER/REPO/issues/123 in your browser.\n", }, { - name: "non-interactive editor", + name: "updating last issue comment with non-interactive web opens issue in browser focusing on new comment if there are no comments", + input: &shared.CommentableOptions{ + Interactive: false, + InputType: shared.InputTypeWeb, + Body: "", + + OpenInBrowser: func(string) error { return nil }, + }, + emptyComments: true, + stderr: "Opening https://github.com/OWNER/REPO/issues/123 in your browser.\n", + }, + { + name: "creating new issue comment with non-interactive editor succeeds", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -344,7 +348,20 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", }, { - name: "non-interactive editor with edit last", + name: "updating last issue comment with non-interactive editor fails if there are no comments", + input: &shared.CommentableOptions{ + Interactive: false, + InputType: shared.InputTypeEditor, + Body: "", + EditLast: true, + + EditSurvey: func(string) (string, error) { return "comment body", nil }, + }, + emptyComments: true, + wantsErr: true, + }, + { + name: "updating last issue comment with non-interactive editor succeeds if there are comments", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -356,10 +373,11 @@ func Test_commentRun(t *testing.T) { httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockCommentUpdate(t, reg) }, - stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-111\n", + emptyComments: false, + stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-111\n", }, { - name: "non-interactive editor with edit last and create if none", + name: "updating last issue comment with non-interactive editor creates new issue if there are no comments but --create-if-none", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -376,7 +394,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", }, { - name: "non-interactive inline", + name: "creating new issue comment with non-interactive succeeds if comment body is provided", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -388,7 +406,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", }, { - name: "non-interactive inline with edit last", + name: "updating last issue comment with non-interactive succeeds if there are comments and comment body is provided", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -398,7 +416,8 @@ func Test_commentRun(t *testing.T) { httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockCommentUpdate(t, reg) }, - stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-111\n", + emptyComments: false, + stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-111\n", }, } for _, tt := range tests { @@ -437,6 +456,10 @@ func Test_commentRun(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := shared.CommentableRun(tt.input) + if tt.wantsErr { + assert.Error(t, err) + return + } assert.NoError(t, err) assert.Equal(t, tt.stdout, stdout.String()) assert.Equal(t, tt.stderr, stderr.String()) diff --git a/pkg/cmd/pr/shared/commentable.go b/pkg/cmd/pr/shared/commentable.go index 46e3072e2..c10c4befa 100644 --- a/pkg/cmd/pr/shared/commentable.go +++ b/pkg/cmd/pr/shared/commentable.go @@ -92,27 +92,38 @@ func CommentableRun(opts *CommentableOptions) error { return err } opts.Host = repo.RepoHost() - if opts.EditLast { - err := updateComment(commentable, opts) - if !errors.Is(err, errNoUserComments) { + + // Create new comment, bail before complexities of updating the last comment + if !opts.EditLast { + return createComment(commentable, opts) + } + + // Update the last comment, handling success or unexpected errors accordingly + err = updateComment(commentable, opts) + if err == nil { + return nil + } + if !errors.Is(err, errNoUserComments) { + return err + } + + // Determine whether to create new comment, prompt user if interactive and missing option + if !opts.CreateIfNone && opts.Interactive { + opts.CreateIfNone, err = opts.ConfirmCreateIfNoneSurvey() + if err != nil { return err } - - if opts.Interactive { - if opts.CreateIfNone { - fmt.Fprintln(opts.IO.ErrOut, "No comments found. Creating a new comment.") - } else { - ok, err := opts.ConfirmCreateIfNoneSurvey() - if err != nil { - return err - } - if !ok { - return errNoUserComments - } - } - } } - return createComment(commentable, opts) + + // Create new comment because updating the last comment failed due to no user comments + if opts.CreateIfNone { + if opts.Interactive { + fmt.Fprintln(opts.IO.ErrOut, "No comments found. Creating a new comment.") + } + return createComment(commentable, opts) + } + + return errNoUserComments } func createComment(commentable Commentable, opts *CommentableOptions) error { From f4382d1345cf68a5e000a4f63c4e0f61531827cd Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 18 Mar 2025 13:13:06 +0100 Subject: [PATCH 20/25] Revert "Merge pull request #10384 from iamazeem/9798-gh-api-encode-package-name" This reverts commit ed2c322a73f150fd424f4fd21b95edab7d129a2e, reversing changes made to f019cf7cea157f40e1f08bcf5573ffa9f0e796ec. --- pkg/cmd/api/api.go | 37 ----------------------- pkg/cmd/api/api_test.go | 66 ----------------------------------------- 2 files changed, 103 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index d25b4ce64..586aeae93 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -7,7 +7,6 @@ import ( "fmt" "io" "net/http" - "net/url" "os" "path/filepath" "regexp" @@ -265,8 +264,6 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command return err } - opts.RequestPath = escapePackageNameInPath(opts.RequestPath) - if runF != nil { return runF(&opts) } @@ -694,37 +691,3 @@ func previewNamesToMIMETypes(names []string) string { } return strings.Join(types, ", ") } - -// The package name part in the `packages` endpoints may contain slashes and -// other characters that need to be URL encoded. -// -// The `escapePackageNameInPath` function extracts and normalizes package names -// in the path. The regex `pathWithPackageNameRE` is being used to extract the -// package name with a capture group named `package`. -// -// See https://docs.github.com/en/rest/packages/packages APIs for more details. -// -// Here's an example: -// -// The package name `orders/cache` needs to be URL encoded because it contains -// a slash `/`. The `escapePackageNameInPath` function will extract the -// `orders/cache` part, perform the URL encoding, and return the normalized API -// endpoint with `%2F` replacing the slash `/` in the package name part only. -// -// - Package name: `orders/cache` -// - API endpoint: `/users/USER/packages/container/orders/cache` -// - Normalized: `/users/USER/packages/container/orders%2Fcache` - -var pathWithPackageNameRE = regexp.MustCompile(`^\/(?:orgs|user|users)(?:\/.*)?\/packages\/(?:npm|maven|rubygems|docker|nuget|container)\/(?.*?)(?:\/(?:restore|versions)|$)`) - -func escapePackageNameInPath(path string) string { - matches := pathWithPackageNameRE.FindStringSubmatch(path) - if len(matches) > 0 { - i := pathWithPackageNameRE.SubexpIndex("package") - packageName := matches[i] - if packageName != "" { - return strings.Replace(path, packageName, url.QueryEscape(packageName), 1) - } - } - return path -} diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index ee58d55d0..321f7b7c0 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -367,72 +367,6 @@ func Test_NewCmdApi(t *testing.T) { }, wantsErr: false, }, - { - name: "request path with container package name containing slashes", - cli: "/user/packages/container/github.com/username/package_name --verbose", - wants: ApiOptions{ - Hostname: "", - RequestMethod: "GET", - RequestMethodPassed: false, - RequestPath: "/user/packages/container/github.com%2Fusername%2Fpackage_name", - RequestInputFile: "", - RawFields: []string(nil), - MagicFields: []string(nil), - RequestHeaders: []string(nil), - ShowResponseHeaders: false, - Paginate: false, - Silent: false, - CacheTTL: 0, - Template: "", - FilterOutput: "", - Verbose: true, - }, - wantsErr: false, - }, - { - name: "request path with container package name containing slashes and restore", - cli: "/user/packages/container/github.com/username/package_name/restore --verbose", - wants: ApiOptions{ - Hostname: "", - RequestMethod: "GET", - RequestMethodPassed: false, - RequestPath: "/user/packages/container/github.com%2Fusername%2Fpackage_name/restore", - RequestInputFile: "", - RawFields: []string(nil), - MagicFields: []string(nil), - RequestHeaders: []string(nil), - ShowResponseHeaders: false, - Paginate: false, - Silent: false, - CacheTTL: 0, - Template: "", - FilterOutput: "", - Verbose: true, - }, - wantsErr: false, - }, - { - name: "request path with container package name containing slashes and versions", - cli: "/user/packages/container/github.com/username/package_name/versions --verbose", - wants: ApiOptions{ - Hostname: "", - RequestMethod: "GET", - RequestMethodPassed: false, - RequestPath: "/user/packages/container/github.com%2Fusername%2Fpackage_name/versions", - RequestInputFile: "", - RawFields: []string(nil), - MagicFields: []string(nil), - RequestHeaders: []string(nil), - ShowResponseHeaders: false, - Paginate: false, - Silent: false, - CacheTTL: 0, - Template: "", - FilterOutput: "", - Verbose: true, - }, - wantsErr: false, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From a47651df43c53c74e8c0c2a725edd5940e87b927 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 18 Mar 2025 15:56:25 -0400 Subject: [PATCH 21/25] Bring issue/PR comment tests up to par, correct This commit brings the `gh issue comment` and `gh pr comment` tests in line with one another while also addressing some corner cases that weren't previously tested. --- pkg/cmd/issue/comment/comment_test.go | 36 +++++--- pkg/cmd/pr/comment/comment_test.go | 120 ++++++++++++++++---------- 2 files changed, 101 insertions(+), 55 deletions(-) diff --git a/pkg/cmd/issue/comment/comment_test.go b/pkg/cmd/issue/comment/comment_test.go index c7feaf698..55b15d740 100644 --- a/pkg/cmd/issue/comment/comment_test.go +++ b/pkg/cmd/issue/comment/comment_test.go @@ -308,30 +308,29 @@ func Test_commentRun(t *testing.T) { { name: "updating last issue comment with non-interactive web opens issue in browser focusing on the last comment", input: &shared.CommentableOptions{ - Interactive: false, - InputType: shared.InputTypeWeb, - Body: "", - EditLast: true, - CreateIfNone: true, + Interactive: false, + InputType: shared.InputTypeWeb, + Body: "", + EditLast: true, OpenInBrowser: func(u string) error { assert.Contains(t, u, "#issuecomment-111") return nil }, }, - stderr: "Opening https://github.com/OWNER/REPO/issues/123 in your browser.\n", + emptyComments: false, + stderr: "Opening https://github.com/OWNER/REPO/issues/123 in your browser.\n", }, { - name: "updating last issue comment with non-interactive web opens issue in browser focusing on new comment if there are no comments", + name: "updating last issue comment with non-interactive web errors because there are no comments", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeWeb, Body: "", - - OpenInBrowser: func(string) error { return nil }, + EditLast: true, }, emptyComments: true, - stderr: "Opening https://github.com/OWNER/REPO/issues/123 in your browser.\n", + wantsErr: true, }, { name: "creating new issue comment with non-interactive editor succeeds", @@ -377,7 +376,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-111\n", }, { - name: "updating last issue comment with non-interactive editor creates new issue if there are no comments but --create-if-none", + name: "updating last issue comment with non-interactive editor creates new comment if there are no comments but --create-if-none", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -419,6 +418,21 @@ func Test_commentRun(t *testing.T) { emptyComments: false, stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-111\n", }, + { + name: "updating last issue comment with non-interactive creates new comment if there are no comments but --create-if-none", + input: &shared.CommentableOptions{ + Interactive: false, + InputType: shared.InputTypeInline, + Body: "comment body", + EditLast: true, + CreateIfNone: true, + }, + emptyComments: true, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + mockCommentCreate(t, reg) + }, + stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", + }, } for _, tt := range tests { ios, _, stdout, stderr := iostreams.Test() diff --git a/pkg/cmd/pr/comment/comment_test.go b/pkg/cmd/pr/comment/comment_test.go index a6cc36abf..1c668a805 100644 --- a/pkg/cmd/pr/comment/comment_test.go +++ b/pkg/cmd/pr/comment/comment_test.go @@ -243,9 +243,10 @@ func Test_commentRun(t *testing.T) { httpStubs func(*testing.T, *httpmock.Registry) stdout string stderr string + wantsErr bool }{ { - name: "interactive editor", + name: "creating new PR comment with interactive editor succeeds", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -260,7 +261,22 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", }, { - name: "interactive editor with edit last", + name: "updating last PR comment with interactive editor fails if there are no comments and decline prompt to create", + input: &shared.CommentableOptions{ + Interactive: true, + InputType: 0, + Body: "", + EditLast: true, + + InteractiveEditSurvey: func(string) (string, error) { return "comment body", nil }, + ConfirmSubmitSurvey: func() (bool, error) { return true, nil }, + ConfirmCreateIfNoneSurvey: func() (bool, error) { return false, nil }, + }, + emptyComments: true, + wantsErr: true, + }, + { + name: "updating last PR comment with interactive editor succeeds if there are comments", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -273,10 +289,11 @@ func Test_commentRun(t *testing.T) { httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockCommentUpdate(t, reg) }, - stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-111\n", + emptyComments: false, + stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-111\n", }, { - name: "interactive editor with edit last and create if none", + name: "updating last PR comment with interactive editor creates new comment if there are no comments but --create-if-none", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -289,12 +306,14 @@ func Test_commentRun(t *testing.T) { ConfirmSubmitSurvey: func() (bool, error) { return true, nil }, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockCommentUpdate(t, reg) + mockCommentCreate(t, reg) }, - stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-111\n", + emptyComments: true, + stderr: "No comments found. Creating a new comment.\n", + stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", }, { - name: "non-interactive web", + name: "creating new PR comment with non-interactive web opens issue in browser focusing on new comment", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeWeb, @@ -302,10 +321,11 @@ func Test_commentRun(t *testing.T) { OpenInBrowser: func(string) error { return nil }, }, - stderr: "Opening https://github.com/OWNER/REPO/pull/123 in your browser.\n", + emptyComments: true, + stderr: "Opening https://github.com/OWNER/REPO/pull/123 in your browser.\n", }, { - name: "non-interactive web with edit last", + name: "updating last PR comment with non-interactive web opens issue in browser focusing on the last comment", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeWeb, @@ -317,43 +337,22 @@ func Test_commentRun(t *testing.T) { return nil }, }, - stderr: "Opening https://github.com/OWNER/REPO/pull/123 in your browser.\n", - }, - { - name: "non-interactive web with edit last and create if none for empty comments", - input: &shared.CommentableOptions{ - Interactive: false, - InputType: shared.InputTypeWeb, - Body: "", - EditLast: true, - CreateIfNone: true, - - OpenInBrowser: func(u string) error { - assert.Contains(t, u, "#issuecomment-new") - return nil - }, - }, - emptyComments: true, + emptyComments: false, stderr: "Opening https://github.com/OWNER/REPO/pull/123 in your browser.\n", }, { - name: "non-interactive web with edit last and create if none", + name: "updating last PR comment with non-interactive web errors because there are no comments", input: &shared.CommentableOptions{ - Interactive: false, - InputType: shared.InputTypeWeb, - Body: "", - EditLast: true, - CreateIfNone: true, - - OpenInBrowser: func(u string) error { - assert.Contains(t, u, "#issuecomment-111") - return nil - }, + Interactive: false, + InputType: shared.InputTypeWeb, + Body: "", + EditLast: true, }, - stderr: "Opening https://github.com/OWNER/REPO/pull/123 in your browser.\n", + emptyComments: true, + wantsErr: true, }, { - name: "non-interactive editor", + name: "creating new PR comment with non-interactive editor succeeds", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -367,7 +366,20 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", }, { - name: "non-interactive editor with edit last", + name: "updating last PR comment with non-interactive editor fails if there are no comments", + input: &shared.CommentableOptions{ + Interactive: false, + InputType: shared.InputTypeEditor, + Body: "", + EditLast: true, + + EditSurvey: func(string) (string, error) { return "comment body", nil }, + }, + emptyComments: true, + wantsErr: true, + }, + { + name: "updating last PR comment with non-interactive editor succeeds if there are comments", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -382,7 +394,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-111\n", }, { - name: "non-interactive editor with edit last and create if none", + name: "updating last PR comment with non-interactive editor creates new comment if there are no comments but --create-if-none", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -399,7 +411,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", }, { - name: "non-interactive inline", + name: "creating new PR comment with non-interactive inline succeeds if comment body is provided", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -411,7 +423,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", }, { - name: "non-interactive inline with edit last", + name: "updating last PR comment with non-interactive inline succeeds if there are comments and comment body is provided", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -421,7 +433,23 @@ func Test_commentRun(t *testing.T) { httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockCommentUpdate(t, reg) }, - stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-111\n", + emptyComments: false, + stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-111\n", + }, + { + name: "updating last PR comment with non-interactive inline creates new comment if there are no comments but --create-if-none", + input: &shared.CommentableOptions{ + Interactive: false, + InputType: shared.InputTypeInline, + Body: "comment body", + EditLast: true, + CreateIfNone: true, + }, + emptyComments: true, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + mockCommentCreate(t, reg) + }, + stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", }, } for _, tt := range tests { @@ -459,6 +487,10 @@ func Test_commentRun(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := shared.CommentableRun(tt.input) + if tt.wantsErr { + assert.Error(t, err) + return + } assert.NoError(t, err) assert.Equal(t, tt.stdout, stdout.String()) assert.Equal(t, tt.stderr, stderr.String()) From bec527d49ee7541a4609ece653c17d4bb4da1f32 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 18 Mar 2025 16:14:14 -0400 Subject: [PATCH 22/25] Clean up pr/issue comment test names --- pkg/cmd/issue/comment/comment_test.go | 28 +++++++++++++-------------- pkg/cmd/pr/comment/comment_test.go | 28 +++++++++++++-------------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/pkg/cmd/issue/comment/comment_test.go b/pkg/cmd/issue/comment/comment_test.go index 55b15d740..794dafda4 100644 --- a/pkg/cmd/issue/comment/comment_test.go +++ b/pkg/cmd/issue/comment/comment_test.go @@ -226,7 +226,7 @@ func Test_commentRun(t *testing.T) { wantsErr bool }{ { - name: "creating new issue comment with interactive editor succeeds", + name: "creating new comment with interactive editor succeeds", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -242,7 +242,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", }, { - name: "updating last issue comment with interactive editor fails if there are no comments and decline prompt to create", + name: "updating last comment with interactive editor fails if there are no comments and decline prompt to create", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -257,7 +257,7 @@ func Test_commentRun(t *testing.T) { wantsErr: true, }, { - name: "updating last issue comment with interactive editor succeeds if there are comments", + name: "updating last comment with interactive editor succeeds if there are comments", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -274,7 +274,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-111\n", }, { - name: "updating last issue comment with interactive editor creates new comment if there are no comments but --create-if-none", + name: "updating last comment with interactive editor creates new comment if there are no comments but --create-if-none", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -294,7 +294,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", }, { - name: "creating new issue comment with non-interactive web opens issue in browser focusing on new comment", + name: "creating new comment with non-interactive web opens issue in browser focusing on new comment", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeWeb, @@ -306,7 +306,7 @@ func Test_commentRun(t *testing.T) { stderr: "Opening https://github.com/OWNER/REPO/issues/123 in your browser.\n", }, { - name: "updating last issue comment with non-interactive web opens issue in browser focusing on the last comment", + name: "updating last comment with non-interactive web opens issue in browser focusing on the last comment", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeWeb, @@ -322,7 +322,7 @@ func Test_commentRun(t *testing.T) { stderr: "Opening https://github.com/OWNER/REPO/issues/123 in your browser.\n", }, { - name: "updating last issue comment with non-interactive web errors because there are no comments", + name: "updating last comment with non-interactive web errors because there are no comments", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeWeb, @@ -333,7 +333,7 @@ func Test_commentRun(t *testing.T) { wantsErr: true, }, { - name: "creating new issue comment with non-interactive editor succeeds", + name: "creating new comment with non-interactive editor succeeds", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -347,7 +347,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", }, { - name: "updating last issue comment with non-interactive editor fails if there are no comments", + name: "updating last comment with non-interactive editor fails if there are no comments", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -360,7 +360,7 @@ func Test_commentRun(t *testing.T) { wantsErr: true, }, { - name: "updating last issue comment with non-interactive editor succeeds if there are comments", + name: "updating last comment with non-interactive editor succeeds if there are comments", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -376,7 +376,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-111\n", }, { - name: "updating last issue comment with non-interactive editor creates new comment if there are no comments but --create-if-none", + name: "updating last comment with non-interactive editor creates new comment if there are no comments but --create-if-none", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -393,7 +393,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", }, { - name: "creating new issue comment with non-interactive succeeds if comment body is provided", + name: "creating new comment with non-interactive inline succeeds if comment body is provided", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -405,7 +405,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", }, { - name: "updating last issue comment with non-interactive succeeds if there are comments and comment body is provided", + name: "updating last comment with non-interactive inline succeeds if there are comments and comment body is provided", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -419,7 +419,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-111\n", }, { - name: "updating last issue comment with non-interactive creates new comment if there are no comments but --create-if-none", + name: "updating last comment with non-interactive inline creates new comment if there are no comments but --create-if-none", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, diff --git a/pkg/cmd/pr/comment/comment_test.go b/pkg/cmd/pr/comment/comment_test.go index 1c668a805..0941f2533 100644 --- a/pkg/cmd/pr/comment/comment_test.go +++ b/pkg/cmd/pr/comment/comment_test.go @@ -246,7 +246,7 @@ func Test_commentRun(t *testing.T) { wantsErr bool }{ { - name: "creating new PR comment with interactive editor succeeds", + name: "creating new comment with interactive editor succeeds", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -261,7 +261,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", }, { - name: "updating last PR comment with interactive editor fails if there are no comments and decline prompt to create", + name: "updating last comment with interactive editor fails if there are no comments and decline prompt to create", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -276,7 +276,7 @@ func Test_commentRun(t *testing.T) { wantsErr: true, }, { - name: "updating last PR comment with interactive editor succeeds if there are comments", + name: "updating last comment with interactive editor succeeds if there are comments", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -293,7 +293,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-111\n", }, { - name: "updating last PR comment with interactive editor creates new comment if there are no comments but --create-if-none", + name: "updating last comment with interactive editor creates new comment if there are no comments but --create-if-none", input: &shared.CommentableOptions{ Interactive: true, InputType: 0, @@ -313,7 +313,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", }, { - name: "creating new PR comment with non-interactive web opens issue in browser focusing on new comment", + name: "creating new comment with non-interactive web opens pull request in browser focusing on new comment", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeWeb, @@ -325,7 +325,7 @@ func Test_commentRun(t *testing.T) { stderr: "Opening https://github.com/OWNER/REPO/pull/123 in your browser.\n", }, { - name: "updating last PR comment with non-interactive web opens issue in browser focusing on the last comment", + name: "updating last comment with non-interactive web opens pull request in browser focusing on the last comment", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeWeb, @@ -341,7 +341,7 @@ func Test_commentRun(t *testing.T) { stderr: "Opening https://github.com/OWNER/REPO/pull/123 in your browser.\n", }, { - name: "updating last PR comment with non-interactive web errors because there are no comments", + name: "updating last comment with non-interactive web errors because there are no comments", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeWeb, @@ -352,7 +352,7 @@ func Test_commentRun(t *testing.T) { wantsErr: true, }, { - name: "creating new PR comment with non-interactive editor succeeds", + name: "creating new comment with non-interactive editor succeeds", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -366,7 +366,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", }, { - name: "updating last PR comment with non-interactive editor fails if there are no comments", + name: "updating last comment with non-interactive editor fails if there are no comments", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -379,7 +379,7 @@ func Test_commentRun(t *testing.T) { wantsErr: true, }, { - name: "updating last PR comment with non-interactive editor succeeds if there are comments", + name: "updating last comment with non-interactive editor succeeds if there are comments", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -394,7 +394,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-111\n", }, { - name: "updating last PR comment with non-interactive editor creates new comment if there are no comments but --create-if-none", + name: "updating last comment with non-interactive editor creates new comment if there are no comments but --create-if-none", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeEditor, @@ -411,7 +411,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", }, { - name: "creating new PR comment with non-interactive inline succeeds if comment body is provided", + name: "creating new comment with non-interactive inline succeeds if comment body is provided", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -423,7 +423,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", }, { - name: "updating last PR comment with non-interactive inline succeeds if there are comments and comment body is provided", + name: "updating last comment with non-interactive inline succeeds if there are comments and comment body is provided", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -437,7 +437,7 @@ func Test_commentRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-111\n", }, { - name: "updating last PR comment with non-interactive inline creates new comment if there are no comments but --create-if-none", + name: "updating last comment with non-interactive inline creates new comment if there are no comments but --create-if-none", input: &shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, From bbb8213e9c5476b73e3245c888a7d6c4136916b9 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 19 Mar 2025 07:56:53 -0400 Subject: [PATCH 23/25] Refactor commentable logic --- pkg/cmd/pr/shared/commentable.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/pr/shared/commentable.go b/pkg/cmd/pr/shared/commentable.go index c10c4befa..f909c7559 100644 --- a/pkg/cmd/pr/shared/commentable.go +++ b/pkg/cmd/pr/shared/commentable.go @@ -114,16 +114,16 @@ func CommentableRun(opts *CommentableOptions) error { return err } } - - // Create new comment because updating the last comment failed due to no user comments - if opts.CreateIfNone { - if opts.Interactive { - fmt.Fprintln(opts.IO.ErrOut, "No comments found. Creating a new comment.") - } - return createComment(commentable, opts) + if !opts.CreateIfNone { + return errNoUserComments } - return errNoUserComments + // Create new comment because updating the last comment failed due to no user comments + if opts.Interactive { + fmt.Fprintln(opts.IO.ErrOut, "No comments found. Creating a new comment.") + } + + return createComment(commentable, opts) } func createComment(commentable Commentable, opts *CommentableOptions) error { From c2ba5c01990cbfda95c4c957f9ddd1ba2ebb617c Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 19 Mar 2025 08:05:51 -0400 Subject: [PATCH 24/25] Curly bracket wrapping acceptance tests --- .../issue-comment-edit-last-with-comments.txtar | 16 ++++++++-------- ...ment-edit-last-without-comments-creates.txtar | 12 ++++++------ ...mment-edit-last-without-comments-errors.txtar | 10 +++++----- .../testdata/issue/issue-comment-new.txtar | 12 ++++++------ .../pr/pr-comment-edit-last-with-comments.txtar | 16 ++++++++-------- ...ment-edit-last-without-comments-creates.txtar | 12 ++++++------ ...mment-edit-last-without-comments-errors.txtar | 10 +++++----- acceptance/testdata/pr/pr-comment-new.txtar | 12 ++++++------ 8 files changed, 50 insertions(+), 50 deletions(-) diff --git a/acceptance/testdata/issue/issue-comment-edit-last-with-comments.txtar b/acceptance/testdata/issue/issue-comment-edit-last-with-comments.txtar index d63c2e17a..a30286ce3 100644 --- a/acceptance/testdata/issue/issue-comment-edit-last-with-comments.txtar +++ b/acceptance/testdata/issue/issue-comment-edit-last-with-comments.txtar @@ -2,30 +2,30 @@ env REPO=${SCRIPT_NAME}-${RANDOM_STRING} # Create a repository with a file so it has a default branch -exec gh repo create $ORG/$REPO --add-readme --private +exec gh repo create ${ORG}/${REPO} --add-readme --private # Defer repo cleanup -defer gh repo delete --yes $ORG/$REPO +defer gh repo delete --yes ${ORG}/${REPO} # Clone the repo -exec gh repo clone $ORG/$REPO +exec gh repo clone ${ORG}/${REPO} # Create an issue in the repo -cd $REPO +cd ${REPO} exec gh issue create --title 'Feature Request' --body 'Feature Body' stdout2env ISSUE_URL # Comment on the issue -exec gh issue comment $ISSUE_URL --body 'Looks like a great feature!' +exec gh issue comment ${ISSUE_URL} --body 'Looks like a great feature!' # View the issue -exec gh issue view $ISSUE_URL --comments +exec gh issue view ${ISSUE_URL} --comments stdout 'Looks like a great feature!' # Edit the last comment -exec gh issue comment $ISSUE_URL --edit-last --body 'Looks like an amazing feature!' +exec gh issue comment ${ISSUE_URL} --edit-last --body 'Looks like an amazing feature!' # View the issue -exec gh issue view $ISSUE_URL --comments +exec gh issue view ${ISSUE_URL} --comments ! stdout 'Looks like a great feature!' stdout 'Looks like an amazing feature!' diff --git a/acceptance/testdata/issue/issue-comment-edit-last-without-comments-creates.txtar b/acceptance/testdata/issue/issue-comment-edit-last-without-comments-creates.txtar index 6dc09d4db..44532680b 100644 --- a/acceptance/testdata/issue/issue-comment-edit-last-without-comments-creates.txtar +++ b/acceptance/testdata/issue/issue-comment-edit-last-without-comments-creates.txtar @@ -2,22 +2,22 @@ env REPO=${SCRIPT_NAME}-${RANDOM_STRING} # Create a repository with a file so it has a default branch -exec gh repo create $ORG/$REPO --add-readme --private +exec gh repo create ${ORG}/${REPO} --add-readme --private # Defer repo cleanup -defer gh repo delete --yes $ORG/$REPO +defer gh repo delete --yes ${ORG}/${REPO} # Clone the repo -exec gh repo clone $ORG/$REPO +exec gh repo clone ${ORG}/${REPO} # Create an issue in the repo -cd $REPO +cd ${REPO} exec gh issue create --title 'Feature Request' --body 'Feature Body' stdout2env ISSUE_URL # Comment on the issue -exec gh issue comment $ISSUE_URL --edit-last --create-if-none --body 'Looks like a great feature!' +exec gh issue comment ${ISSUE_URL} --edit-last --create-if-none --body 'Looks like a great feature!' # View the issue -exec gh issue view $ISSUE_URL --comments +exec gh issue view ${ISSUE_URL} --comments stdout 'Looks like a great feature!' diff --git a/acceptance/testdata/issue/issue-comment-edit-last-without-comments-errors.txtar b/acceptance/testdata/issue/issue-comment-edit-last-without-comments-errors.txtar index 050ff92d5..300131180 100644 --- a/acceptance/testdata/issue/issue-comment-edit-last-without-comments-errors.txtar +++ b/acceptance/testdata/issue/issue-comment-edit-last-without-comments-errors.txtar @@ -2,19 +2,19 @@ env REPO=${SCRIPT_NAME}-${RANDOM_STRING} # Create a repository with a file so it has a default branch -exec gh repo create $ORG/$REPO --add-readme --private +exec gh repo create ${ORG}/${REPO} --add-readme --private # Defer repo cleanup -defer gh repo delete --yes $ORG/$REPO +defer gh repo delete --yes ${ORG}/${REPO} # Clone the repo -exec gh repo clone $ORG/$REPO +exec gh repo clone ${ORG}/${REPO} # Create an issue in the repo -cd $REPO +cd ${REPO} exec gh issue create --title 'Feature Request' --body 'Feature Body' stdout2env ISSUE_URL # Comment on the issue -! exec gh issue comment $ISSUE_URL --edit-last --body 'Looks like a great feature!' +! exec gh issue comment ${ISSUE_URL} --edit-last --body 'Looks like a great feature!' stderr 'no comments found for current user' diff --git a/acceptance/testdata/issue/issue-comment-new.txtar b/acceptance/testdata/issue/issue-comment-new.txtar index 2ce318629..12524b6d5 100644 --- a/acceptance/testdata/issue/issue-comment-new.txtar +++ b/acceptance/testdata/issue/issue-comment-new.txtar @@ -2,22 +2,22 @@ env REPO=${SCRIPT_NAME}-${RANDOM_STRING} # Create a repository with a file so it has a default branch -exec gh repo create $ORG/$REPO --add-readme --private +exec gh repo create ${ORG}/${REPO} --add-readme --private # Defer repo cleanup -defer gh repo delete --yes $ORG/$REPO +defer gh repo delete --yes ${ORG}/${REPO} # Clone the repo -exec gh repo clone $ORG/$REPO +exec gh repo clone ${ORG}/${REPO} # Create an issue in the repo -cd $REPO +cd ${REPO} exec gh issue create --title 'Feature Request' --body 'Feature Body' stdout2env ISSUE_URL # Comment on the issue -exec gh issue comment $ISSUE_URL --body 'Looks like a great feature!' +exec gh issue comment ${ISSUE_URL} --body 'Looks like a great feature!' # View the issue -exec gh issue view $ISSUE_URL --comments +exec gh issue view ${ISSUE_URL} --comments stdout 'Looks like a great feature!' diff --git a/acceptance/testdata/pr/pr-comment-edit-last-with-comments.txtar b/acceptance/testdata/pr/pr-comment-edit-last-with-comments.txtar index 5b371978c..be4650ffa 100644 --- a/acceptance/testdata/pr/pr-comment-edit-last-with-comments.txtar +++ b/acceptance/testdata/pr/pr-comment-edit-last-with-comments.txtar @@ -5,16 +5,16 @@ env REPO=${SCRIPT_NAME}-${RANDOM_STRING} exec gh auth setup-git # Create a repository with a file so it has a default branch -exec gh repo create $ORG/$REPO --add-readme --private +exec gh repo create ${ORG}/${REPO} --add-readme --private # Defer repo cleanup -defer gh repo delete --yes $ORG/$REPO +defer gh repo delete --yes ${ORG}/${REPO} # Clone the repo -exec gh repo clone $ORG/$REPO +exec gh repo clone ${ORG}/${REPO} # Prepare a branch to PR -cd $REPO +cd ${REPO} exec git checkout -b feature-branch exec git commit --allow-empty -m 'Empty Commit' exec git push -u origin feature-branch @@ -25,16 +25,16 @@ exec gh pr create --title 'Feature Title' --body 'Feature Body' stdout2env PR_URL # Comment on the PR -exec gh pr comment $PR_URL --body 'Looks like a great feature!' +exec gh pr comment ${PR_URL} --body 'Looks like a great feature!' # View the PR -exec gh pr view $PR_URL --comments +exec gh pr view ${PR_URL} --comments stdout 'Looks like a great feature!' # Edit the last comment -exec gh pr comment $PR_URL --edit-last --body 'Looks like an amazing feature!' +exec gh pr comment ${PR_URL} --edit-last --body 'Looks like an amazing feature!' # View the PR -exec gh pr view $PR_URL --comments +exec gh pr view ${PR_URL} --comments ! stdout 'Looks like a great feature!' stdout 'Looks like an amazing feature!' diff --git a/acceptance/testdata/pr/pr-comment-edit-last-without-comments-creates.txtar b/acceptance/testdata/pr/pr-comment-edit-last-without-comments-creates.txtar index 4f2039f9d..e6205737f 100644 --- a/acceptance/testdata/pr/pr-comment-edit-last-without-comments-creates.txtar +++ b/acceptance/testdata/pr/pr-comment-edit-last-without-comments-creates.txtar @@ -5,16 +5,16 @@ env REPO=${SCRIPT_NAME}-${RANDOM_STRING} exec gh auth setup-git # Create a repository with a file so it has a default branch -exec gh repo create $ORG/$REPO --add-readme --private +exec gh repo create ${ORG}/${REPO} --add-readme --private # Defer repo cleanup -defer gh repo delete --yes $ORG/$REPO +defer gh repo delete --yes ${ORG}/${REPO} # Clone the repo -exec gh repo clone $ORG/$REPO +exec gh repo clone ${ORG}/${REPO} # Prepare a branch to PR -cd $REPO +cd ${REPO} exec git checkout -b feature-branch exec git commit --allow-empty -m 'Empty Commit' exec git push -u origin feature-branch @@ -25,8 +25,8 @@ exec gh pr create --title 'Feature Title' --body 'Feature Body' stdout2env PR_URL # Comment on the PR -exec gh pr comment $PR_URL --edit-last --create-if-none --body 'Looks like a great feature!' +exec gh pr comment ${PR_URL} --edit-last --create-if-none --body 'Looks like a great feature!' # View the PR -exec gh pr view $PR_URL --comments +exec gh pr view ${PR_URL} --comments stdout 'Looks like a great feature!' diff --git a/acceptance/testdata/pr/pr-comment-edit-last-without-comments-errors.txtar b/acceptance/testdata/pr/pr-comment-edit-last-without-comments-errors.txtar index 8f28db5cd..3a70adb72 100644 --- a/acceptance/testdata/pr/pr-comment-edit-last-without-comments-errors.txtar +++ b/acceptance/testdata/pr/pr-comment-edit-last-without-comments-errors.txtar @@ -5,16 +5,16 @@ env REPO=${SCRIPT_NAME}-${RANDOM_STRING} exec gh auth setup-git # Create a repository with a file so it has a default branch -exec gh repo create $ORG/$REPO --add-readme --private +exec gh repo create ${ORG}/${REPO} --add-readme --private # Defer repo cleanup -defer gh repo delete --yes $ORG/$REPO +defer gh repo delete --yes ${ORG}/${REPO} # Clone the repo -exec gh repo clone $ORG/$REPO +exec gh repo clone ${ORG}/${REPO} # Prepare a branch to PR -cd $REPO +cd ${REPO} exec git checkout -b feature-branch exec git commit --allow-empty -m 'Empty Commit' exec git push -u origin feature-branch @@ -25,5 +25,5 @@ exec gh pr create --title 'Feature Title' --body 'Feature Body' stdout2env PR_URL # Comment on the PR -! exec gh pr comment $PR_URL --edit-last --body 'Looks like a great feature!' +! exec gh pr comment ${PR_URL} --edit-last --body 'Looks like a great feature!' stderr 'no comments found for current user' diff --git a/acceptance/testdata/pr/pr-comment-new.txtar b/acceptance/testdata/pr/pr-comment-new.txtar index 9856c0430..c5b80314c 100644 --- a/acceptance/testdata/pr/pr-comment-new.txtar +++ b/acceptance/testdata/pr/pr-comment-new.txtar @@ -5,16 +5,16 @@ env REPO=${SCRIPT_NAME}-${RANDOM_STRING} exec gh auth setup-git # Create a repository with a file so it has a default branch -exec gh repo create $ORG/$REPO --add-readme --private +exec gh repo create ${ORG}/${REPO} --add-readme --private # Defer repo cleanup -defer gh repo delete --yes $ORG/$REPO +defer gh repo delete --yes ${ORG}/${REPO} # Clone the repo -exec gh repo clone $ORG/$REPO +exec gh repo clone ${ORG}/${REPO} # Prepare a branch to PR -cd $REPO +cd ${REPO} exec git checkout -b feature-branch exec git commit --allow-empty -m 'Empty Commit' exec git push -u origin feature-branch @@ -24,8 +24,8 @@ exec gh pr create --title 'Feature Title' --body 'Feature Body' stdout2env PR_URL # Comment on the PR -exec gh pr comment $PR_URL --body 'Looks like a great feature!' +exec gh pr comment ${PR_URL} --body 'Looks like a great feature!' # View the PR -exec gh pr view $PR_URL --comments +exec gh pr view ${PR_URL} --comments stdout 'Looks like a great feature!' From c7cd041a4057e82ad859dd8495ecbf2e97705474 Mon Sep 17 00:00:00 2001 From: Ryan Winograd Date: Sat, 22 Mar 2025 15:21:24 -0500 Subject: [PATCH 25/25] Fix typos in CONTRIBUTING.md --- .github/CONTRIBUTING.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 4e2032a87..a1ed27d99 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -62,9 +62,9 @@ To propose a design: - Include a link to the issue that the design is for. - Describe the design you are proposing to resolve the issue, leveraging the [CLI Design System][]. - Mock up the design you are proposing using our [Google Docs Template][] or code blocks. - - Mock ups should cleary illustrate the command(s) being run and the expected output(s). + - Mock ups should clearly illustrate the command(s) being run and the expected output(s). -### (core team only) Revewing a design +### (core team only) Reviewing a design A member of the core team will [triage](../docs/triage.md) the design proposal. Once a member of the core team has reviewed the design, they may add the [`help wanted`][hw] label to the issue, so a PR can be opened to provide the implementation.