diff --git a/CHANGELOG.md b/CHANGELOG.md index ecf1c6aa..91d63a08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ on the default behavior. ### Bugfix - Upgrade golang and project dependencies to the latest version +- Change use of deprecated print `::set-output` to write to `$GITHUB_OUTPUT` file ## v1.2.0 - 2024-08-09 diff --git a/internal/testutil/gha_output.go b/internal/testutil/gha_output.go new file mode 100644 index 00000000..5de27813 --- /dev/null +++ b/internal/testutil/gha_output.go @@ -0,0 +1,40 @@ +package testutil + +import ( + "io" + "os" + "path" + "testing" + + "github.com/newrelic/release-toolkit/src/app/gha" +) + +type GithubOutputWriter struct { + File *os.File +} + +func NewGithubOutputWriter(t *testing.T) GithubOutputWriter { + t.Helper() + + ghaOutputFileName := path.Join(t.TempDir(), "temporary_github_output_file") + t.Setenv(gha.GithubOutput, ghaOutputFileName) + + ghaOutputFile, err := os.Create(ghaOutputFileName) + if err != nil { + t.Fatalf("Error creating temporary GHA output file for test: %v", err) + } + + return GithubOutputWriter{ + File: ghaOutputFile, + } +} + +func (ghaOut GithubOutputWriter) Result(t *testing.T) string { + t.Helper() + + actual, err := io.ReadAll(ghaOut.File) + if err != nil { + t.Fatalf("Unable to read temporary GHA output file: %v", err) + } + return string(actual) +} diff --git a/src/app/generate/generate.go b/src/app/generate/generate.go index c0d44166..d743be5e 100644 --- a/src/app/generate/generate.go +++ b/src/app/generate/generate.go @@ -119,7 +119,10 @@ type appendDepSrc func([]changelog.Source, git.TagsVersionGetter, git.CommitsGet // //nolint:gocyclo,cyclop func Generate(cCtx *cli.Context) error { - gh := gha.NewFromCli(cCtx) + gh, err := gha.NewFromCli(cCtx) + if err != nil { + return fmt.Errorf("creating github client: %w", err) + } yamlPath := cCtx.String(common.YAMLFlag) chFile, err := os.Create(yamlPath) diff --git a/src/app/generate/generate_test.go b/src/app/generate/generate_test.go index 147f435a..7ad51c81 100644 --- a/src/app/generate/generate_test.go +++ b/src/app/generate/generate_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/newrelic/release-toolkit/internal/testutil" "github.com/newrelic/release-toolkit/src/app" "github.com/newrelic/release-toolkit/src/git" ) @@ -37,18 +38,18 @@ This is a release note //nolint:funlen,paralleltest,maintidx func TestGenerate(t *testing.T) { for _, tc := range []struct { - name string - commits []string - author string - md string - expected string - outputExpected string - args string - preCmdArgs string + name string + commits []string + author string + md string + expected string + expectedStdOutput string + expectedGHAOutput string + args string + preCmdArgs string }{ { name: "Empty_Changelog_gha", - args: "--exit-code=0", preCmdArgs: "--gha=1", md: strings.TrimSpace(` # Changelog @@ -61,7 +62,8 @@ This is based on blah blah blah ### Enhancements - This is in the past and should not be included `), - outputExpected: "::set-output name=empty-changelog::true\n", + expectedStdOutput: "", + expectedGHAOutput: "empty-changelog=true\n", expected: strings.TrimSpace(` notes: "" changes: [] @@ -427,6 +429,7 @@ dependencies: } { //nolint:paralleltest t.Run(tc.name, func(t *testing.T) { + ghaOutput := testutil.NewGithubOutputWriter(t) tDir := repoWithCommits(t, tc.author, tc.commits...) tc.expected = calculateHashes(t, tDir, tc.expected) @@ -460,8 +463,11 @@ dependencies: if diff := cmp.Diff(tc.expected, string(yaml)); diff != "" { t.Fatalf("Output YAML is not as expected:\n%s", diff) } - if actual := buf.String(); actual != tc.outputExpected { - t.Fatalf("Expected %q, app printed: %q", tc.outputExpected, actual) + if actual := buf.String(); actual != tc.expectedStdOutput { + t.Fatalf("Expected %q, app printed: %q", tc.expectedStdOutput, actual) + } + if actual := ghaOutput.Result(t); actual != tc.expectedGHAOutput { + t.Fatalf("Expected %q, GHA output: %q", tc.expectedStdOutput, actual) } }) } diff --git a/src/app/gha/gha.go b/src/app/gha/gha.go index 112bc045..27b32265 100644 --- a/src/app/gha/gha.go +++ b/src/app/gha/gha.go @@ -3,13 +3,22 @@ package gha import ( + "errors" "fmt" "io" + "os" + "sync" "github.com/newrelic/release-toolkit/src/app/common" "github.com/urfave/cli/v2" ) +var ErrEmptyFileName = errors.New("filename is an empty string") + +const ( + GithubOutput = "GITHUB_OUTPUT" +) + // New creates a Github object that will print commands to the specified writer. func New(writer io.Writer) Github { return Github{w: writer} @@ -17,21 +26,35 @@ func New(writer io.Writer) Github { // NewFromCli takes a cli.Context and looks for common.GHAFlag on it. If it is set, it returns a Github object that // writes to the app's Writer. If it is not, it returns an empty Github object that does not write anything. -func NewFromCli(cCtx *cli.Context) Github { - if cCtx.Bool(common.GHAFlag) { - return New(cCtx.App.Writer) +func NewFromCli(cCtx *cli.Context) (Github, error) { + if !cCtx.Bool(common.GHAFlag) { + return New(io.Discard), nil + } + + filename := os.Getenv(GithubOutput) + + if filename == "" { + return Github{}, fmt.Errorf("invalid output file: %w", ErrEmptyFileName) + } + + file, err := os.OpenFile(filename, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) //nolint:mnd,magicnumber + if err != nil { + return Github{}, fmt.Errorf("invalid output file: %w", err) } - return New(io.Discard) + return New(file), nil } // Github is an object that output workflow commands. type Github struct { - w io.Writer + w io.Writer + mu sync.Mutex } // SetOutput outputs the `set-output` command. -// https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-an-output-parameter -func (g Github) SetOutput(name string, value interface{}) { - _, _ = fmt.Fprintf(g.w, "::set-output name=%s::%v\n", name, value) +// https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#setting-an-output-parameter +func (g *Github) SetOutput(name string, value interface{}) { + g.mu.Lock() + _, _ = fmt.Fprintf(g.w, "%s=%v\n", name, value) + g.mu.Unlock() } diff --git a/src/app/gha/gha_test.go b/src/app/gha/gha_test.go new file mode 100644 index 00000000..6e05a0a0 --- /dev/null +++ b/src/app/gha/gha_test.go @@ -0,0 +1,72 @@ +package gha_test + +import ( + "strings" + "sync" + "testing" + + "github.com/newrelic/release-toolkit/internal/testutil" + "github.com/newrelic/release-toolkit/src/app/gha" +) + +//nolint:paralleltest +func TestGHA_FileIsEmptyByDefault(t *testing.T) { + ghaOutput := testutil.NewGithubOutputWriter(t) + if actual := ghaOutput.Result(t); actual != "" { + t.Fatalf("Expected GHA output is empty") + } +} + +func TestGHA_OutputsAreAppended(t *testing.T) { + t.Parallel() + + buf := &strings.Builder{} + buf.WriteString("not-empty=true\nanother-line=true\n") + + gha := gha.New(buf) + + gha.SetOutput("test", 1) + gha.SetOutput("test", "out") + + expected := strings.TrimSpace(` +not-empty=true +another-line=true +test=1 +test=out + `) + "\n" + + if actual := buf.String(); actual != expected { + t.Fatalf("Expected:\n%s\n\ngot:\n%s", expected, actual) + } +} + +func TestGHA_LockWritesAndDoesNotMangleOutput(t *testing.T) { + t.Parallel() + + buf := &strings.Builder{} + wg := sync.WaitGroup{} + + gha := gha.New(buf) + + for range 5 { + wg.Add(1) + go func() { + gha.SetOutput("test", 1) + wg.Done() + }() + } + + expected := strings.TrimSpace(` +test=1 +test=1 +test=1 +test=1 +test=1 + `) + "\n" + + wg.Wait() + + if actual := buf.String(); actual != expected { + t.Fatalf("Expected:\n%s\n\ngot:\n%s", expected, actual) + } +} diff --git a/src/app/isempty/isempty.go b/src/app/isempty/isempty.go index 00064465..fc806fa5 100644 --- a/src/app/isempty/isempty.go +++ b/src/app/isempty/isempty.go @@ -33,7 +33,10 @@ var Cmd = &cli.Command{ // IsEmpty is a command function which loads a changelog.yaml file, and prints to stdout whether it is empty or not. func IsEmpty(cCtx *cli.Context) error { - gh := gha.NewFromCli(cCtx) + gh, err := gha.NewFromCli(cCtx) + if err != nil { + return fmt.Errorf("creating github client: %w", err) + } chPath := cCtx.String(common.YAMLFlag) chFile, err := os.Open(chPath) diff --git a/src/app/isempty/isempty_test.go b/src/app/isempty/isempty_test.go index 012cb64d..94a98a8b 100644 --- a/src/app/isempty/isempty_test.go +++ b/src/app/isempty/isempty_test.go @@ -2,6 +2,7 @@ package isempty_test import ( "fmt" + "io" "os" "path" "strings" @@ -9,7 +10,9 @@ import ( "github.com/urfave/cli/v2" + "github.com/newrelic/release-toolkit/internal/testutil" "github.com/newrelic/release-toolkit/src/app" + "github.com/newrelic/release-toolkit/src/app/gha" ) //nolint:paralleltest,gocyclo,cyclop @@ -32,24 +35,27 @@ changes: [] `) for _, tc := range []struct { - cmd string - expected string - errExpected bool + cmd string + expectedStdOutput string + expectedGHAOutput string + errExpected bool }{ { - cmd: fmt.Sprintf("rt -yaml %s is-empty", filepath), - expected: "true\n", - errExpected: false, + cmd: fmt.Sprintf("rt -yaml %s is-empty", filepath), + expectedStdOutput: "true\n", + errExpected: false, }, { - cmd: fmt.Sprintf("rt -gha=1 -yaml %s is-empty", filepath), - expected: "true\n::set-output name=is-empty::true\n", - errExpected: false, + cmd: fmt.Sprintf("rt -gha=1 -yaml %s is-empty", filepath), + expectedStdOutput: "true\n", + expectedGHAOutput: "is-empty=true\n", + errExpected: false, }, { - cmd: fmt.Sprintf("rt -gha=1 -yaml %s is-empty -fail", filepath), - expected: "true\n::set-output name=is-empty::true\n", - errExpected: true, + cmd: fmt.Sprintf("rt -gha=1 -yaml %s is-empty -fail", filepath), + expectedStdOutput: "true\n", + expectedGHAOutput: "is-empty=true\n", + errExpected: true, }, } { var errValue int @@ -57,6 +63,14 @@ changes: [] errValue = code } + ghaOutputFileName := path.Join(t.TempDir(), "temporary_github_output_file") + t.Setenv(gha.GithubOutput, ghaOutputFileName) + + ghaOutputFile, err := os.Create(ghaOutputFileName) + if err != nil { + t.Fatalf("Error creating temporary GHA output file for test: %v", err) + } + buf.Reset() err = app.Run(strings.Fields(tc.cmd)) if err != nil && !tc.errExpected { @@ -66,8 +80,8 @@ changes: [] t.Fatalf("An error was expected running app: %v", err) } - if actual := buf.String(); actual != tc.expected { - t.Fatalf("Expected %q, app printed: %q", tc.expected, actual) + if actual := buf.String(); actual != tc.expectedStdOutput { + t.Fatalf("Expected %q, app printed: %q", tc.expectedStdOutput, actual) } if errValue != 1 && tc.errExpected { @@ -77,6 +91,14 @@ changes: [] if errValue == 1 && !tc.errExpected { t.Fatalf("An exit code 1 was not expected") } + + actual, err := io.ReadAll(ghaOutputFile) + if err != nil { + t.Fatalf("Unable to read temporary GHA output file: %v", err) + } + if string(actual) != tc.expectedGHAOutput { + t.Fatalf("Expected %q, GHA output: %q", tc.expectedStdOutput, actual) + } } } @@ -101,16 +123,18 @@ changes: `) for _, tc := range []struct { - cmd string - expected string + cmd string + expectedStdOutput string + expectedGHAOutput string }{ { - cmd: fmt.Sprintf("rt -yaml %s is-empty -fail", filepath), - expected: "false\n", + cmd: fmt.Sprintf("rt -yaml %s is-empty -fail", filepath), + expectedStdOutput: "false\n", }, { - cmd: fmt.Sprintf("rt -gha=1 -yaml %s is-empty -fail", filepath), - expected: "false\n::set-output name=is-empty::false\n", + cmd: fmt.Sprintf("rt -gha=1 -yaml %s is-empty -fail", filepath), + expectedStdOutput: "false\n", + expectedGHAOutput: "is-empty=false\n", }, } { var errValue int @@ -118,14 +142,19 @@ changes: errValue = code } + ghaOutput := testutil.NewGithubOutputWriter(t) + buf.Reset() err = app.Run(strings.Fields(tc.cmd)) if err != nil { t.Fatalf("Error running app: %v", err) } - if actual := buf.String(); actual != tc.expected { - t.Fatalf("Expected %q, app printed: %q", tc.expected, actual) + if actual := buf.String(); actual != tc.expectedStdOutput { + t.Fatalf("Expected %q, app printed: %q", tc.expectedStdOutput, actual) + } + if actual := ghaOutput.Result(t); actual != tc.expectedGHAOutput { + t.Fatalf("Expected %q, GHA output: %q", tc.expectedStdOutput, actual) } if errValue != 0 { diff --git a/src/app/isheld/isheld.go b/src/app/isheld/isheld.go index c1cc1956..76f0b8f8 100644 --- a/src/app/isheld/isheld.go +++ b/src/app/isheld/isheld.go @@ -35,7 +35,10 @@ var Cmd = &cli.Command{ // IsHeld is a command function which loads a changelog.yaml file from this, and prints to stdout whether it has the // Held flag set to true. func IsHeld(cCtx *cli.Context) error { - gh := gha.NewFromCli(cCtx) + gh, err := gha.NewFromCli(cCtx) + if err != nil { + return fmt.Errorf("creating github client: %w", err) + } chPath := cCtx.String(common.YAMLFlag) chFile, err := os.Open(chPath) diff --git a/src/app/isheld/isheld_test.go b/src/app/isheld/isheld_test.go index 5d2839dd..135689fc 100644 --- a/src/app/isheld/isheld_test.go +++ b/src/app/isheld/isheld_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + "github.com/newrelic/release-toolkit/internal/testutil" "github.com/newrelic/release-toolkit/src/app" ) @@ -32,26 +33,32 @@ changes: `) for _, tc := range []struct { - cmd string - expected string + cmd string + expectedStdOutput string + expectedGHAOutput string }{ { - cmd: fmt.Sprintf("rt -yaml %s is-held", filepath), - expected: "true\n", + cmd: fmt.Sprintf("rt -yaml %s is-held", filepath), + expectedStdOutput: "true\n", }, { - cmd: fmt.Sprintf("rt -gha=1 -yaml %s is-held", filepath), - expected: "true\n::set-output name=is-held::true\n", + cmd: fmt.Sprintf("rt -gha=1 -yaml %s is-held", filepath), + expectedStdOutput: "true\n", + expectedGHAOutput: "is-held=true\n", }, } { + ghaOutput := testutil.NewGithubOutputWriter(t) buf.Reset() err = app.Run(strings.Fields(tc.cmd)) if err != nil { t.Fatalf("Error running app: %v", err) } - if actual := buf.String(); actual != tc.expected { - t.Fatalf("Expected %q, app printed: %q", tc.expected, actual) + if actual := buf.String(); actual != tc.expectedStdOutput { + t.Fatalf("Expected %q, app printed: %q", tc.expectedStdOutput, actual) + } + if actual := ghaOutput.Result(t); actual != tc.expectedGHAOutput { + t.Fatalf("Expected %q, GHA output: %q", tc.expectedStdOutput, actual) } } } diff --git a/src/app/nextversion/nextversion.go b/src/app/nextversion/nextversion.go index 4769607a..0a96375c 100644 --- a/src/app/nextversion/nextversion.go +++ b/src/app/nextversion/nextversion.go @@ -105,7 +105,10 @@ next-version will exit with an error if no previous versions are found in the gi // //nolint:gocyclo,cyclop func NextVersion(cCtx *cli.Context) error { - gh := gha.NewFromCli(cCtx) + gh, err := gha.NewFromCli(cCtx) + if err != nil { + return fmt.Errorf("creating github client: %w", err) + } nextOverride, err := parseNextFlag(cCtx.String(nextFlag)) if err != nil { diff --git a/src/app/nextversion/nextversion_test.go b/src/app/nextversion/nextversion_test.go index 2b920c2c..f70f0b88 100644 --- a/src/app/nextversion/nextversion_test.go +++ b/src/app/nextversion/nextversion_test.go @@ -9,6 +9,7 @@ import ( "strings" "testing" + "github.com/newrelic/release-toolkit/internal/testutil" "github.com/newrelic/release-toolkit/src/app" "github.com/newrelic/release-toolkit/src/bump" "github.com/newrelic/release-toolkit/src/bumper" @@ -17,17 +18,18 @@ import ( //nolint:paralleltest, funlen // urfave/cli cannot be tested concurrently. func TestNextVersion_Without_Repo(t *testing.T) { for _, tc := range []struct { - name string - yaml string - expected string - errorExpected error - args string - globalargs string + name string + yaml string + expectedStdOutput string + expectedGHAOutput string + errorExpected error + args string + globalargs string }{ { - name: "Overrides_Next_And_Current", - args: "-next v0.0.1 -current v1.2.3", - expected: "v0.0.1", + name: "Overrides_Next_And_Current", + args: "-next v0.0.1 -current v1.2.3", + expectedStdOutput: "v0.0.1", yaml: strings.TrimSpace(` notes: |- ### Important announcement (note) @@ -47,9 +49,9 @@ dependencies: `), }, { - name: "Bumps_Patch", - args: "-current v1.2.3", - expected: "v1.2.4", + name: "Bumps_Patch", + args: "-current v1.2.3", + expectedStdOutput: "v1.2.4", yaml: strings.TrimSpace(` changes: - type: bugfix @@ -57,9 +59,9 @@ changes: `), }, { - name: "Bumps_Minor", - args: "-current v1.2.3", - expected: "v1.3.0", + name: "Bumps_Minor", + args: "-current v1.2.3", + expectedStdOutput: "v1.3.0", yaml: strings.TrimSpace(` changes: - type: enhancement @@ -67,9 +69,9 @@ changes: `), }, { - name: "Bumps_Major", - args: "-current v1.2.3", - expected: "v2.0.0", + name: "Bumps_Major", + args: "-current v1.2.3", + expectedStdOutput: "v2.0.0", yaml: strings.TrimSpace(` changes: - type: breaking @@ -77,10 +79,11 @@ changes: `), }, { - name: "Bumps_Major_GHA", - globalargs: "-gha=true", - args: "-current v1.2.3", - expected: "v2.0.0\n::set-output name=next-version::v2.0.0\n::set-output name=next-version-major::v2\n::set-output name=next-version-major-minor::v2.0", + name: "Bumps_Major_GHA", + globalargs: "-gha=true", + args: "-current v1.2.3", + expectedStdOutput: "v2.0.0", + expectedGHAOutput: "next-version=v2.0.0\nnext-version-major=v2\nnext-version-major-minor=v2.0\n", yaml: strings.TrimSpace(` changes: - type: breaking @@ -88,9 +91,9 @@ changes: `), }, { - name: "No_Bump_without_failing", - args: "-current v1.2.3 -fail=false", - expected: "v1.2.3", + name: "No_Bump_without_failing", + args: "-current v1.2.3 -fail=false", + expectedStdOutput: "v1.2.3", yaml: strings.TrimSpace(` changes: [] `), @@ -108,6 +111,7 @@ changes: [] //nolint:paralleltest // urfave/cli cannot be tested concurrently. t.Run(tc.name, func(t *testing.T) { tDir := t.TempDir() + ghaOutput := testutil.NewGithubOutputWriter(t) app := app.App() @@ -127,9 +131,11 @@ changes: [] t.Fatalf("Expected error %v, got %v", tc.errorExpected, err) } - actual := buf.String() - if tc.expected != "" && actual != tc.expected+"\n" { - t.Fatalf("Expected %q, got %q", tc.expected, actual) + if actual := buf.String(); tc.expectedStdOutput != "" && actual != tc.expectedStdOutput+"\n" { + t.Fatalf("Expected %q, App printed: %q", tc.expectedStdOutput, actual) + } + if actual := ghaOutput.Result(t); actual != tc.expectedGHAOutput { + t.Fatalf("Expected %q, GHA output: %q", tc.expectedStdOutput, actual) } }) } diff --git a/src/app/validate/validate.go b/src/app/validate/validate.go index 80b3860a..7db006bf 100644 --- a/src/app/validate/validate.go +++ b/src/app/validate/validate.go @@ -42,7 +42,10 @@ var Cmd = &cli.Command{ // Validate is a command function which loads a changelog.md file, and prints to stderr // all the errors found. func Validate(cCtx *cli.Context) error { - gh := gha.NewFromCli(cCtx) + gh, err := gha.NewFromCli(cCtx) + if err != nil { + return fmt.Errorf("creating github client: %w", err) + } mdPath := cCtx.String(markdownPathFlag) chFile, err := os.Open(mdPath) diff --git a/src/app/validate/validate_test.go b/src/app/validate/validate_test.go index af5e1706..268fc931 100644 --- a/src/app/validate/validate_test.go +++ b/src/app/validate/validate_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + "github.com/newrelic/release-toolkit/internal/testutil" "github.com/newrelic/release-toolkit/src/app" ) @@ -74,7 +75,7 @@ Support has been removed "Important announcement (note)" header found with empty content "Breaking" header must contain only an itemized list `, "\n"), - expectedGha: "::set-output name=valid::false\n", + expectedGha: "valid=false\n", }, { name: "Valid_Changelog", @@ -126,7 +127,7 @@ This is a release note `), args: "--exit-code=0", expectedErr: "", - expectedGha: "::set-output name=valid::true\n", + expectedGha: "valid=true\n", }, { name: "Invalid_Changelog_Only_Notes", @@ -154,12 +155,11 @@ unreleased changelog can't only contain notes //nolint:paralleltest // urfave/cli cannot be tested concurrently. t.Run(tc.name, func(t *testing.T) { tDir := t.TempDir() + ghaOutput := testutil.NewGithubOutputWriter(t) app := app.App() bufErr := &strings.Builder{} - buf := &strings.Builder{} app.ErrWriter = bufErr - app.Writer = buf mdPath := path.Join(tDir, "CHANGELOG.md") mdFile, err := os.Create(mdPath) @@ -178,8 +178,7 @@ unreleased changelog can't only contain notes if actual := bufErr.String(); actual != tc.expectedErr { t.Fatalf("Expected:\n%s\n\napp printed:\n%s", tc.expectedErr, actual) } - - if actual := buf.String(); actual != tc.expectedGha { + if actual := ghaOutput.Result(t); actual != tc.expectedGha { t.Fatalf("Expected:\n%s\n\napp printed:\n%s", tc.expectedGha, actual) } })