Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace deprecated GHA ::set-output syntax #980

Merged

Conversation

phil9909
Copy link
Member

Summary

Fixes #867

Use the new way of producing outputs in a github action as the old way is deprecated and will be removed (31st May 2023).

See https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

Use Cases

Pipelines will continue to work after May 31st 2023.

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@phil9909 phil9909 requested a review from a team January 20, 2023 16:30
@dmikusa dmikusa added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels Jan 24, 2023
Copy link
Contributor

@dmikusa dmikusa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, a few questions/possible adjustments.

In the meantime, I'm going to manually PR this into our test pipeline on paketo-buildpacks/pipeline-builder-canary.

if !ok {
panic(errors.New("GITHUB_OUTPUT is not set, see https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-an-output-parameter"))
}
writer, err := os.OpenFile(outputFileName, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0666)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason to not use os.Create(..)? Typically, that is preferred for writing files.

https://pkg.go.dev/os#Create

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.Create passes the O_TRUNC flag, we pass O_APPEND instead.

Basically the same as >> "$GITHUB_OUTPUT" vs > "$GITHUB_OUTPUT" in bash would be. In their examples GitHub uses append.
I assume, if we truncate the file we would delete outputs that were created elsewhere. This might lead to subtle bugs, as GitHub Actions don't fail on missing values in their interpolation syntax.
So IMHO this is the better option, even if it is less readable.

My suggestions:

  • Either I add a comment explaining the difference to os.Open
  • or I move the os.OpenFile call to a new helper function called Append, with a doc comment that it works like os.Open, but uses the append instead of the truncate flag.

What would you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ha. Ok. That makes perfect sense, don't want to truncate the file and wipe out previously written output.

To clarify the code I don't have a strong preference, but given a choice, I'd say let's go with the Append method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I called the method createOrAppend:

  1. I guess it's better private, therefore lowercase
  2. append would collide with the built-in array append function.

actions/action.go Show resolved Hide resolved
actions/compute-artifact-description/main.go Outdated Show resolved Hide resolved
actions/spring-generations/main.go Outdated Show resolved Hide resolved
@dmikusa
Copy link
Contributor

dmikusa commented Jan 24, 2023

A couple of other things...

  1. Please run go generate ./... from the octo/ subdirectory. This will update the statik file, which needs to be updated when the shell scripts are modified.
  2. Please bump mheap/github-action-required-labels@v2 to mheap/github-action-required-labels@v3. That should update that action to a newer version which doesn't use ::set-output.

Thanks

dmikusa added a commit to paketo-buildpacks/pipeline-builder-canary that referenced this pull request Jan 24, 2023
Run `octo` using the commit from paketo-buildpacks/pipeline-builder#980. This is a test to make sure the changes are OK.

Signed-off-by: Daniel Mikusa <dan@mikusa.com>
Co-authored-by: Daniel Mikusa <dan@mikusa.com>
@phil9909 phil9909 force-pushed the replace-deprecated-gha-syntax branch 2 times, most recently from d91eefc to 536a7f4 Compare January 24, 2023 14:56
@phil9909 phil9909 force-pushed the replace-deprecated-gha-syntax branch from 536a7f4 to d603692 Compare January 24, 2023 15:09
@phil9909
Copy link
Member Author

  1. Please run go generate ./... from the octo/ subdirectory. This will update the statik file, which needs to be updated when the shell scripts are modified.
  2. Please bump mheap/github-action-required-labels@v2 to mheap/github-action-required-labels@v3. That should update that action to a newer version which doesn't use ::set-output.

Done

@phil9909 phil9909 requested a review from dmikusa January 24, 2023 15:13
This was referenced Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate from deprecated GHA features
2 participants