From 117e7a3a630a1be0cb7815b643e99d820360e74e Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Tue, 17 May 2022 15:34:42 -0400 Subject: [PATCH] Panic on errors and remove deprecated functions (#41) Previously, if writing to a env/path file failed, the library would silently fail if writing to the file failed. Similarly, if writing to an output stream fails, the library would silently fail. This updates the behavior to panic on these low-level errors. Additionally, this removes deprecated ways of setting environment variables and paths given the new GitHub runners have been available for months. It also removes deprecated library functions. --- .github/workflows/unit.yml | 15 +---- actions.go | 124 +++++++++++++++++++------------------ actions_root.go | 4 +- actions_test.go | 65 +++---------------- 4 files changed, 77 insertions(+), 131 deletions(-) diff --git a/.github/workflows/unit.yml b/.github/workflows/unit.yml index d981d01..e796c1d 100644 --- a/.github/workflows/unit.yml +++ b/.github/workflows/unit.yml @@ -48,19 +48,10 @@ jobs: runs-on: '${{ matrix.os }}' steps: - - uses: 'actions/checkout@v2' + - uses: 'actions/checkout@v3' - - uses: 'actions/setup-go@v2' + - uses: 'actions/setup-go@v3' with: go-version: '${{ matrix.go }}' - - uses: 'actions/cache@v2' - with: - path: '~/go/pkg/mod' - key: '${{ matrix.os }}-go-${{ matrix.go }}-${{ hashFiles(''**/go.sum'') }}' - restore-keys: | - ${{ matrix.os }}-go-${{ matrix.go }} - ${{ matrix.os }}-go- - - - name: 'Test' - run: 'make test-acc' + - run: 'make test-acc' diff --git a/actions.go b/actions.go index c74cebe..fe64af0 100644 --- a/actions.go +++ b/actions.go @@ -84,15 +84,6 @@ func New(opts ...Option) *Action { return a } -// NewWithWriter creates a wrapper using the given writer. This is useful for -// tests. The given writer cannot add any prefixes to the string, since GitHub -// requires these special strings to match a very particular format. -// -// Deprecated: Use New() with WithWriter instead. -func NewWithWriter(w io.Writer) *Action { - return New(WithWriter(w)) -} - // Action is an internal wrapper around GitHub Actions' output and magic // strings. type Action struct { @@ -102,12 +93,16 @@ type Action struct { httpClient *http.Client } -// IssueCommand issues a new GitHub actions Command. +// IssueCommand issues a new GitHub actions Command. It panics if it cannot +// write to the output stream. func (c *Action) IssueCommand(cmd *Command) { - fmt.Fprint(c.w, cmd.String()+EOF) + if _, err := fmt.Fprint(c.w, cmd.String()+EOF); err != nil { + panic(fmt.Errorf("failed to issue command: %w", err)) + } } // IssueFileCommand issues a new GitHub actions Command using environment files. +// It panics if writing to the file fails. // // https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-commands-for-github-actions#environment-files // @@ -118,7 +113,15 @@ func (c *Action) IssueCommand(cmd *Command) { // IssueFileCommand currently ignores the 'CommandProperties' field provided // with the 'Command' argument as it's scope is unclear in the current // TypeScript implementation. -func (c *Action) IssueFileCommand(cmd *Command) error { +func (c *Action) IssueFileCommand(cmd *Command) { + if err := c.issueFileCommand(cmd); err != nil { + panic(err) + } +} + +// issueFileCommand is an internal-only helper that issues the command and +// returns an error to make testing easier. +func (c *Action) issueFileCommand(cmd *Command) (retErr error) { e := strings.ReplaceAll(cmd.Name, "-", "_") e = strings.ToUpper(e) e = "GITHUB_" + e @@ -127,17 +130,26 @@ func (c *Action) IssueFileCommand(cmd *Command) error { msg := []byte(cmd.Message + EOF) f, err := os.OpenFile(filepath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) if err != nil { - return fmt.Errorf(errFileCmdFmt, err) + retErr = fmt.Errorf(errFileCmdFmt, err) + return } - defer f.Close() + + defer func() { + if err := f.Close(); err != nil && retErr == nil { + retErr = err + } + }() + if _, err := f.Write(msg); err != nil { - return fmt.Errorf(errFileCmdFmt, err) + retErr = fmt.Errorf(errFileCmdFmt, err) + return } - return nil + return } // AddMask adds a new field mask for the given string "p". After called, future -// attempts to log "p" will be replaced with "***" in log output. +// attempts to log "p" will be replaced with "***" in log output. It panics if +// it cannot write to the output stream. func (c *Action) AddMask(p string) { // ::add-mask::

c.IssueCommand(&Command{ @@ -146,7 +158,8 @@ func (c *Action) AddMask(p string) { }) } -// AddMatcher adds a new matcher with the given file path. +// AddMatcher adds a new matcher with the given file path. It panics if it +// cannot write to the output stream. func (c *Action) AddMatcher(p string) { // ::add-matcher::

c.IssueCommand(&Command{ @@ -155,7 +168,8 @@ func (c *Action) AddMatcher(p string) { }) } -// RemoveMatcher removes a matcher with the given owner name. +// RemoveMatcher removes a matcher with the given owner name. It panics if it +// cannot write to the output stream. func (c *Action) RemoveMatcher(o string) { // ::remove-matcher owner=:: c.IssueCommand(&Command{ @@ -166,30 +180,20 @@ func (c *Action) RemoveMatcher(o string) { }) } -// AddPath adds the string "p" to the path for the invocation. It attempts to -// issue a file command at first. If that fails, it falls back to the regular -// (now deprecated) 'add-path' command, which may stop working in the future. -// The deprecated fallback may be useful for users running an older version of -// GitHub runner. +// AddPath adds the string "p" to the path for the invocation. It panics if it +// cannot write to the output file. // // https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-commands-for-github-actions#adding-a-system-path // https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ func (c *Action) AddPath(p string) { - err := c.IssueFileCommand(&Command{ + c.IssueFileCommand(&Command{ Name: pathCmd, Message: p, }) - - if err != nil { // use regular command as fallback - // ::add-path::

- c.IssueCommand(&Command{ - Name: addPathCmd, - Message: p, - }) - } } -// SaveState saves state to be used in the "finally" post job entry point. +// SaveState saves state to be used in the "finally" post job entry point. It +// panics if it cannot write to the output stream. func (c *Action) SaveState(k, v string) { // ::save-state name=:: c.IssueCommand(&Command{ @@ -201,7 +205,8 @@ func (c *Action) SaveState(k, v string) { }) } -// GetInput gets the input by the given name. +// GetInput gets the input by the given name. It returns the empty string if the +// input is not defined. func (c *Action) GetInput(i string) string { e := strings.ReplaceAll(i, " ", "_") e = strings.ToUpper(e) @@ -209,7 +214,8 @@ func (c *Action) GetInput(i string) string { return strings.TrimSpace(c.getenv(e)) } -// Group starts a new collapsable region up to the next ungroup invocation. +// Group starts a new collapsable region up to the next ungroup invocation. It +// panics if it cannot write to the output stream. func (c *Action) Group(t string) { // ::group:: c.IssueCommand(&Command{ @@ -218,7 +224,8 @@ func (c *Action) Group(t string) { }) } -// EndGroup ends the current group. +// EndGroup ends the current group. It panics if it cannot write to the output +// stream. func (c *Action) EndGroup() { // ::endgroup:: c.IssueCommand(&Command{ @@ -226,32 +233,20 @@ func (c *Action) EndGroup() { }) } -// SetEnv sets an environment variable. It attempts to issue a file command at -// first. If that fails, it falls back to the regular (now deprecated) 'set-env' -// command, which may stop working in the future. The deprecated fallback may be -// useful for users running an older version of GitHub runner. +// SetEnv sets an environment variable. It panics if it cannot write to the +// output file. // // https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-commands-for-github-actions#setting-an-environment-variable // https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ func (c *Action) SetEnv(k, v string) { - err := c.IssueFileCommand(&Command{ + c.IssueFileCommand(&Command{ Name: envCmd, Message: fmt.Sprintf(envCmdMsgFmt, k, envCmdDelimiter, v, envCmdDelimiter), }) - - if err != nil { // use regular command as fallback - // ::set-env name=:: - c.IssueCommand(&Command{ - Name: setEnvCmd, - Message: v, - Properties: CommandProperties{ - "name": k, - }, - }) - } } -// SetOutput sets an output parameter. +// SetOutput sets an output parameter. It panics if it cannot write to the +// output stream. func (c *Action) SetOutput(k, v string) { // ::set-output name=:: c.IssueCommand(&Command{ @@ -264,7 +259,8 @@ func (c *Action) SetOutput(k, v string) { } // Debugf prints a debug-level message. It follows the standard fmt.Printf -// arguments, appending an OS-specific line break to the end of the message. +// arguments, appending an OS-specific line break to the end of the message. It +// panics if it cannot write to the output stream. func (c *Action) Debugf(msg string, args ...interface{}) { // ::debug :: c.IssueCommand(&Command{ @@ -275,7 +271,8 @@ func (c *Action) Debugf(msg string, args ...interface{}) { } // Noticef prints a notice-level message. It follows the standard fmt.Printf -// arguments, appending an OS-specific line break to the end of the message. +// arguments, appending an OS-specific line break to the end of the message. It +// panics if it cannot write to the output stream. func (c *Action) Noticef(msg string, args ...interface{}) { // ::notice :: c.IssueCommand(&Command{ @@ -286,7 +283,8 @@ func (c *Action) Noticef(msg string, args ...interface{}) { } // Warningf prints a warning-level message. It follows the standard fmt.Printf -// arguments, appending an OS-specific line break to the end of the message. +// arguments, appending an OS-specific line break to the end of the message. It +// panics if it cannot write to the output stream. func (c *Action) Warningf(msg string, args ...interface{}) { // ::warning :: c.IssueCommand(&Command{ @@ -297,7 +295,8 @@ func (c *Action) Warningf(msg string, args ...interface{}) { } // Errorf prints a error-level message. It follows the standard fmt.Printf -// arguments, appending an OS-specific line break to the end of the message. +// arguments, appending an OS-specific line break to the end of the message. It +// panics if it cannot write to the output stream. func (c *Action) Errorf(msg string, args ...interface{}) { // ::error :: c.IssueCommand(&Command{ @@ -314,10 +313,13 @@ func (c *Action) Fatalf(msg string, args ...interface{}) { osExit(1) } -// Infof prints message to stdout without any level annotations. It follows the standard fmt.Printf -// arguments, appending an OS-specific line break to the end of the message. +// Infof prints message to stdout without any level annotations. It follows the +// standard fmt.Printf arguments, appending an OS-specific line break to the end +// of the message. It panics if it cannot write to the output stream. func (c *Action) Infof(msg string, args ...interface{}) { - fmt.Fprintf(c.w, msg+EOF, args...) + if _, err := fmt.Fprintf(c.w, msg+EOF, args...); err != nil { + panic(fmt.Errorf("failed to write info command: %w", err)) + } } // WithFieldsSlice includes the provided fields in log output. "f" must be a diff --git a/actions_root.go b/actions_root.go index 88d9ba4..f1e29bd 100644 --- a/actions_root.go +++ b/actions_root.go @@ -26,8 +26,8 @@ func IssueCommand(cmd *Command) { } // IssueFileCommand issues a new GitHub actions Command using environment files. -func IssueFileCommand(cmd *Command) error { - return defaultAction.IssueFileCommand(cmd) +func IssueFileCommand(cmd *Command) { + defaultAction.IssueFileCommand(cmd) } // AddMask adds a new field mask for the given string "p". After called, future diff --git a/actions_test.go b/actions_test.go index 60c2a70..37b1272 100644 --- a/actions_test.go +++ b/actions_test.go @@ -53,22 +53,6 @@ func TestNew(t *testing.T) { } } -func TestNewWithWriter(t *testing.T) { - t.Parallel() - - var b bytes.Buffer - a := NewWithWriter(&b) - - a.IssueCommand(&Command{ - Name: "foo", - Message: "bar", - }) - - if got, want := b.String(), "::foo::bar"+EOF; got != want { - t.Errorf("expected %q to be %q", got, want) - } -} - func TestAction_IssueCommand(t *testing.T) { t.Parallel() @@ -87,7 +71,7 @@ func TestAction_IssueCommand(t *testing.T) { func TestAction_IssueFileCommand(t *testing.T) { t.Parallel() - file, err := ioutil.TempFile(".", ".issue_file_cmd_test_") + file, err := ioutil.TempFile("", "") if err != nil { t.Fatalf("unable to create a temp env file: %s", err) } @@ -98,12 +82,10 @@ func TestAction_IssueFileCommand(t *testing.T) { var b bytes.Buffer a := New(WithWriter(&b), WithGetenv(fakeGetenvFunc)) - err = a.IssueFileCommand(&Command{ + if err := a.issueFileCommand(&Command{ Name: "foo", Message: "bar", - }) - - if err != nil { + }); err != nil { t.Errorf("expected nil error, got: %s", err) } @@ -164,27 +146,16 @@ func TestAction_AddPath(t *testing.T) { const envGitHubPath = "GITHUB_PATH" - // expect a regular command to be issued when env file is not set. - fakeGetenvFunc := newFakeGetenvFunc(t, envGitHubPath, "") - var b bytes.Buffer - a := New(WithWriter(&b), WithGetenv(fakeGetenvFunc)) - - a.AddPath("/custom/bin") - if got, want := b.String(), "::add-path::/custom/bin"+EOF; got != want { - t.Errorf("expected %q to be %q", got, want) - } - - b.Reset() - // expect a file command to be issued when env file is set. - file, err := ioutil.TempFile(".", ".add_path_test_") + file, err := ioutil.TempFile("", "") if err != nil { t.Fatalf("unable to create a temp env file: %s", err) } - defer os.Remove(file.Name()) - fakeGetenvFunc = newFakeGetenvFunc(t, envGitHubPath, file.Name()) - WithGetenv(fakeGetenvFunc)(a) + + fakeGetenvFunc := newFakeGetenvFunc(t, envGitHubPath, file.Name()) + var b bytes.Buffer + a := New(WithWriter(&b), WithGetenv(fakeGetenvFunc)) a.AddPath("/custom/bin") @@ -261,27 +232,9 @@ func TestAction_SetEnv(t *testing.T) { const envGitHubEnv = "GITHUB_ENV" - // expectations for regular set-env commands - checks := []struct { - key, value, want string - }{ - {"key", "value", "::set-env name=key::value" + EOF}, - {"key", "this is 100% a special\n\r value!", "::set-env name=key::this is 100%25 a special%0A%0D value!" + EOF}, - } - - for _, check := range checks { - fakeGetenvFunc := newFakeGetenvFunc(t, envGitHubEnv, "") - var b bytes.Buffer - a := New(WithWriter(&b), WithGetenv(fakeGetenvFunc)) - a.SetEnv(check.key, check.value) - if got, want := b.String(), check.want; got != want { - t.Errorf("SetEnv(%q, %q): expected %q; got %q", check.key, check.value, want, got) - } - } - // expectations for env file env commands var b bytes.Buffer - file, err := ioutil.TempFile(".", ".set_env_test_") + file, err := ioutil.TempFile("", "") if err != nil { t.Fatalf("unable to create a temp env file: %s", err) }