Conversation
Signed-off-by: dongjiang <dongjiang1989@126.com>
mattfarina
left a comment
There was a problem hiding this comment.
.github/env files are not part of the GitHub Actions features. So, the change in this PR didn't actually work.
In this case go-version is an empty string. This is leading to the CI log message:
Warning: go-version input was not specified. The action will try to use pre-installed version.Can you please create a PR where the go version is working.
Signed-off-by: dongjiang <dongjiang1989@126.com>
Thanks @mattfarina |
Signed-off-by: dongjiang <dongjiang1989@126.com>
Signed-off-by: dongjiang <dongjiang1989@126.com>
Signed-off-by: dongjiang <dongjiang1989@126.com>
scottrigby
left a comment
There was a problem hiding this comment.
This all works now. Tested both with CI and locally. Nice job.
Matt's change request here has been fixed #30677 (review)
I'm asking for one small style change request. Revert the one change where you removed extra lines. Let's keep extra lines consistent in that file.
Once that's done, this should be good to go 👍
.github/workflows/build-test.yml
Outdated
| - name: Import environment variables from file | ||
| run: cat ".github/env" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
I think this makes sense. GitHub recommends exporting env vars to GITHUB_ENV here: https://docs.github.com/en/actions/sharing-automations/creating-actions/metadata-syntax-for-github-actions#runsstepsenv. We also are not exporting anything sensitive—noting because these are printed to console, which is fine for all of this.
Signed-off-by: dongjiang <dongjiang2010@gmail.com>
Signed-off-by: dongjiang <dongjiang2010@gmail.com>
Thanks @scottrigby |
scottrigby
left a comment
There was a problem hiding this comment.
LGTM 👍 Thanks @dongjiang1989 for your persistence.
Note for other reviewers: never mind my nit about line breaks around steps. Our GH Actions workflows are already inconsistent from file to file and sometimes within the same file. If we care to format these consistently we can do that in a follow up.
What this PR does / why we need it:
This is needed to update to the latest k8s packages(v1.33.beta.0) as they require Go 1.24
Special notes for your reviewer:
If applicable:
docs neededlabel should be applied if so)