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 #193

Conversation

phil9909
Copy link
Contributor

@phil9909 phil9909 commented Mar 17, 2023

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/

Notice: I did not update the actions managed by the pipeline-builder. These should be fixed by this automated PR #177

@phil9909 phil9909 requested review from a team as code owners March 17, 2023 14:41
_, _ = fmt.Fprintf(d.Writer, "::set-output name=%s::%s\n", name, escape(value))
err := d.export("GITHUB_OUTPUT", name, value)
if err != nil {
panic(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewer:

I decided to use panic here. I don't think an action can recover from such an error.

I'm open to changing this and return an error, but that would mean touching all the call-sides and handle the error (by panic?)

Comment on lines +263 to +270
if d.Delemiter == "" {
data := make([]byte, 16) // roughly the same entropy as uuid v4 used in https://github.com/actions/toolkit/blob/b36e70495fbee083eb20f600eafa9091d832577d/packages/core/src/file-command.ts#L28
_, err := rand.Read(data)
if err != nil {
panic(fmt.Errorf("could not generate random delimiter: %w", err))
}
d.Delemiter = hex.EncodeToString(data)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewer:

GitHub recommends using random delimiters when writing multi-line strings to environment files for security reasons. See https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#multiline-strings

Also: It is not unthinkable that an output contains the word EOF which would cause problems.

The Delimiter member is initialized in the test setup to keep the unit test deterministic.

@phil9909 phil9909 force-pushed the replace-deprecated-gha-syntax branch from 2640cc1 to e0c3baf Compare March 28, 2023 07:14
@jkutner jkutner added type:task A general task semver:patch A change requiring a patch version bump labels Mar 28, 2023
@jkutner
Copy link
Member

jkutner commented Mar 28, 2023

@phil9909 thanks (again). I'll pull this into my own branch if you don't get a chance to update yours by tomorrow

@phil9909 phil9909 force-pushed the replace-deprecated-gha-syntax branch from e0c3baf to 987415a Compare March 29, 2023 05:44
@phil9909
Copy link
Contributor Author

I got the following error in Action buildpackage-verify-metadata / Create Action (pull_request) :

   # github.com/google/go-containerregistry/pkg/name
  /go/pkg/mod/github.com/google/go-containerregistry@v0.14.0/pkg/name/errors.go:38:43: undefined: any
  note: module requires Go 1.18
  # github.com/docker/cli/cli/config/credentials
  /go/pkg/mod/github.com/docker/cli@v23.0.1+incompatible/cli/config/credentials/file_store.go:78:20: undefined: strings.Cut
  # github.com/google/go-containerregistry/pkg/v1/partial
  /go/pkg/mod/github.com/google/go-containerregistry@v0.14.0/pkg/v1/partial/with.go:405:15: undefined: any
  note: module requires Go 1.18

As I couldn't see how this error is related to my changes, I ran docker build --file Dockerfile --build-arg "SOURCE=./buildpackage/verify-metadata/cmd" --tag test:test . on the main branch (ab35423) and it show the same error.

After digging I bit, I guess this happened due to the action not being triggered on updates to go.mod and go.sum using the paths filter

"on":
pull_request:
paths:
- buildpackage/verify-metadata/**
- internal/**
push:
branches:
- main
- test
paths:
- buildpackage/verify-metadata/**
- internal/**
release:
types:
- published

One of the dependabot PRs probably broke the build and my PR is now the first PR touching the code again (and thereby triggering the action).

For now, I will just bump the go version in the Dockerfile (sounds like a good idea anyhow, as go 1.17 is out of maintenance already). But I guess the Actions should be updated to prevent similar problems in the future.

@phil9909 phil9909 force-pushed the replace-deprecated-gha-syntax branch from 1de5ef6 to 3b4c6d3 Compare March 29, 2023 06:15
@jkutner
Copy link
Member

jkutner commented Mar 29, 2023

oof, sorry about that

Signed-off-by: Philipp Stehle <philipp.stehle@sap.com>
Recommended by GitHub for security.
See https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#multiline-strings

Also: It is not unthinkable that an output contains the word `EOF` which would cause problems.

Signed-off-by: Philipp Stehle <philipp.stehle@sap.com>
@phil9909 phil9909 force-pushed the replace-deprecated-gha-syntax branch from 3b4c6d3 to 64337bf Compare March 29, 2023 14:03
@phil9909
Copy link
Contributor Author

phil9909 commented Mar 29, 2023

@jkutner rebased (and dropped the commit that changes the Dockerfile)

@jkutner
Copy link
Member

jkutner commented Mar 29, 2023

@phil9909 thanks. I went ahead and pulled this into #198

@jkutner jkutner closed this Mar 29, 2023
@phil9909 phil9909 deleted the replace-deprecated-gha-syntax branch March 30, 2023 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants