Update version comments for SHA-pinned GitHub Actions#5951
Update version comments for SHA-pinned GitHub Actions#5951jeffwidman merged 4 commits intodependabot:mainfrom
Conversation
|
@jproberts Thanks so much for doing this! The PR looks pretty neat and seems to have extensive test coverage 💪. I'll hope to get to do a proper review of this PR soon-ish unless someone else beat me to it, but I wanted to drop a big THANKS for now. Also pretty nice detail to open a PR to your fork first and get a colleague to review it 🥇. |
jeffwidman
left a comment
There was a problem hiding this comment.
This looks fantastic to me, very well done.
Well done writing this flexibly enough to handle all the permutations people use for specifying version strings.
| - uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # tag=v2.1.0 | ||
| - uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # v2.1.0 | ||
| - uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 #v2.1.0 | ||
| - uses: actions/checkout@01aecc # v2.1.0 |
There was a problem hiding this comment.
Major 💖 for phenom set of test cases.
Two more cases:
- What about the case where a comment includes the version, but then some text?
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # Versions older than v2.1.0 have a security vulnerability
- What if the version string is repeated twice?
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # v2.1.0 - Versions older than v2.1.0 have a security vulnerability
I don't want to throw up roadblocks on this awesome PR, so if for simplicity neither case is bumped, that's perfectly acceptable. So one simple heuristic could be if there's any text after the version string, then don't bump the version string... Users that still want it bumped can instead leave their comment as a full-line comment above the actual code line:
# Versions older than v2.1.0 have a security vulnerability
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # v2.1.0
But we do need to ensure we never auto-bump the comment to start incorrectly saying "Versions older than v2.2.0 have a security vulnerability"...
So can you add these two test cases to ensure no future regressions?
There was a problem hiding this comment.
Thanks for the feedback. I was worried the simple search-and-replace was too broad but I couldn't come up with a sample comment where we wouldn't want to bump the version. The cases you mentioned are exactly what I was looking for!
I've updated the PR with the simple fix to only change comments if there's no text after the version string.
GitHub encourages pinning third-party GitHub Actions to a full
length commit SHA. It's common for actions pinned by commit SHA
to include a comment specifying the version associated with the
commit. For example:
- uses: actions/checkout@01aecc # v2.1.0
This change updates the GitHub Actions manager to bump versions
in comments that follow SHA-pinned actions, so the comment stays
up-to-date with the SHA being updated.
The file_updater now searches the comment string for all references
to the previous version and replaces them with the new version. To
avoid changing unrelated comments, the comment updater only
updates dependencies that pin SHA refs.
If the version in a comment is followed by additional text, it could be documentation explaining some behavior, rather than a tag corresponding to the SHA in the action version. To be safe, don't update the version unless it's the end of the comment.
96b107c to
a459c88
Compare
jeffwidman
left a comment
There was a problem hiding this comment.
Looks great to me.
Had one minor pedantic suggestion about tweaking the description of intended behavior, but I'm also fine to merge as-is... up to you.
We should be able to get this deployed/merged in the next few days...
|
For the CI failure here, it's probably best to just disable the Rubocop line-length check for that block of code: Example of how to do that here: Just don't forget to re-enable it after the block as demonstrated in the above example as well. |
|
FYI @jproberts this got a nice shout out on the blog: https://github.blog/changelog/2022-10-31-dependabot-now-updates-comments-in-github-actions-workflows-referencing-action-versions/ |
|
I just saw the post and I'm amazed. Thanks so much for your support @jeffwidman ❤️ |
|
Congrats on the post! Is this feature compatible with renovatebot's way of handling comments? Renovatebot expects a comment of the form It would be useful for the various update tools to be compatible on this point. Wdut? /cc @rarkins from renovatebot |
|
I have created renovatebot/renovate#18640 recently to align Renovate with this approach. As described in that issue, I have concerns about assuming any comment represents a tag (e.g. |
|
Thanks for swinging by. Agreed that getting the update tools aligned on the implicit format spec would be great. The Thankfully @jproberts was thorough and already included unit tests for all the formats I've seen in the wild: @rarkins you might find the whole set of examples useful to test against, particularly the negative tests. If you come up with additional edge cases we missed, feel free to circle back and let us know. |
|
Thanks for this, saves a lot of time! |
|
Do I need to do anything to opt in to this? Not seeing the version update: https://github.com/argoproj/argo-cd/pull/11480/files |
GitHub encourages pinning third-party GitHub Actions to a full length commit SHA. It's common for actions pinned by commit SHA to include a comment specifying the version associated with the commit. For example:
This change updates the GitHub Actions manager to bump versions in comments that follow SHA-pinned actions, so the comment stays up-to-date with the SHA being updated.
The file_updater now searches the comment string for all references to the previous version and replaces them with the new version. To avoid changing unrelated comments, the comment updater only updates dependencies that pin SHA refs.
Closes #4691