-
Notifications
You must be signed in to change notification settings - Fork 214
Update outdated GitHub Actions #362
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
base: master
Are you sure you want to change the base?
Conversation
Oh, that is a cool contribution. I will take a look, but first I will take a look at the bug introduced with the last version. |
/lgtm |
Is there anything I need to do to get this merged? |
I don't think so, just patience 😅 I was waiting for @stlaz, but he said that it is fine to merge. But there is no hurry yet as there are a couple of other PRs to merge before the next release. |
1a97bd3
to
2a483b3
Compare
@lucacome , actually I just watched a best practice in a talk at KubeCon London: Could you pin the full commit hash of the current checkout v4 (11bd71901bbe5b1630ceea73d27597364c9af683)? It is considered more secure, than following the tag. In context of the whole tj-actions desaster. |
@ibihim I usually have the SHAs for all the Actions in all my repos, but I also add something like dependabot or renovate to keep the dependencies up to date. |
6e8616c
to
21ad02e
Compare
I agree, we will need to change that :) If you have any best practices to share, you are welcome, otherwise I would make it part of the release cycle, to not only bump go deps, but also GitHubActions. |
.github/workflows/build.yml
Outdated
with: | ||
version: ${{ env.kind-version }} | ||
version: "v0.27.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to have this at the top, so one doesn't need to scroll through the file to bump the version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted it. If we decide to use renovate we can keep that up to date automatically too.
The e2e tests are failing :) Sorry, for not approving you earlier, but once I press the "approve" button, your PR gets merged automatically once it is green. |
Can you squash also the commits into one or two (e.g. one for the github action and one for the other stuff)? 😄 |
549db39
to
152f8a7
Compare
I would add renovate. I opened a PR a while ago with dependabot (because you can just add the file to enable it) and it was rejected tho.. 😅 |
a3f9c52
to
0e6f523
Compare
0e6f523
to
2090577
Compare
By the way, I was also planning to add the build step as a GitHub Action instead of the script after this one is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution!
I think we're almost ready to merge. I trust @ibihim on his review, there were just two things I stumbled upon in my quick read, but otherwise the PR looks good to me 👍
uses: engineerd/setup-kind@v0.5.0 | ||
uses: helm/kind-action@a1b0e391336a6ee6713a0583f8c6240d70863de3 # v1.12.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't followed the PR from the beginning, was this change explained somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked it myself. helm is slightly more popular than engineerd (240 stars, last release 10/24, v 0.y.z) vs helm (322 stars, last release 12/2024, v x.y.z). It shouldn't matter too much.
env: | ||
QUAY_PATH: quay.io/brancz/kube-rbac-proxy | ||
go-version: '1.23' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's stick to a stable version rather than running on the stable channel. The decision to move up a version should be explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, ok. Deterministic is better than probabilistic.
But I wish we would live in a world, where we don't need to be too specific.
So we should lock-in x and y? The z-stream should be flexible, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x.y like we had before should be fine
Updates the Actions to the latest version available. This among other things removes the warnings in the runs.
The
setup-go
action now acceptsstable
as a version to always use the latest stable version.Also updated
.golangci.yaml
to remove the warnings about deprecated configuration options.