Fix updating GitHub actions to major tags that are branches #5963
Conversation
deivid-rodriguez
force-pushed
the
deivid-rodriguez/fix-bumping-actions-pinned-to-branches-that-look-like-tags
branch
from
e744c04 to
10b3766
Compare
deivid-rodriguez
force-pushed
the
deivid-rodriguez/fix-bumping-actions-pinned-to-branches-that-look-like-tags
branch
from
10b3766 to
e94bd7e
Compare
|
This one is ready now. It fixes a pretty common case Dependabot could not yet update! 🎉 |
deivid-rodriguez
force-pushed
the
deivid-rodriguez/fix-bumping-actions-pinned-to-branches-that-look-like-tags
branch
2 times, most recently
from
7a3debd to
4d2702b
Compare
| # Otherwise, assume we're pinned | ||
| true | ||
| # If the specified `ref` is actually a branch, we're pinned if the branch looks like a version | ||
| version_tag?(ref) |
There was a problem hiding this comment.
❓ Could there be any unintended consequences of making this change in the common/lib?
The release branch pattern is very common to github-actions and this makes total sense for that ecosystem, but I'm a little concerned that this change in behaviour may mean that in other languages where people are tracking a version branch for other reasons we will start suggesting upgrades where we haven't before 🤔
There was a problem hiding this comment.
🤔 good point!
Seems a very edge case in other ecosystems, and I'm not sure what user's expectation is, but probably they don't want an update? Not really sure...
I made a similar change about a month ago: e1a8c49, by the way 😬.
What should we do? Should I override pinned? with this logic just for Actions?
There was a problem hiding this comment.
I believe e1a8c49 probably makes sense everywhere (and no issues reported so far, which is good).
Something else I'm observing is that this method returns true if an explicit branch argument is given. So I believe this should not affect Bundler, which has an explicit :branch option when it comes to dealing with git sources. In other words, I think this should only potentially affect ecosystems with an "ambiguous notation" for git refs.
There was a problem hiding this comment.
Yeah, I agree on e1a8c49, that seems like a common problem and a logical solution in most cases.
In other words, I think this should only potentially affect ecosystems with an "ambiguous notation" for git refs.
Yeah that's a good way to think of it, I'm not sure offhand which ecosystems this would affect 🤔
It might be best to fix this local action actions to start with as we know it is a definite problem there due to convention and then research making it a general behaviour? My general expectation of using a branch in most languages I've used is that it would track $branch:HEAD as I'm intentionally 'pinning' to a release branch or similar as I can't or don't want to upgrade.
I think it's probably not uncommon to name development branches simply v1 or v2 as well, so I think my instinct is towards making the most focused change possible?
There was a problem hiding this comment.
I pushed 9dd57ba045addcd89d6b6e0235d1700945061071 that I think should restore the previous logic for other ecosystems.
There was a problem hiding this comment.
Hei @brrygrdn! Does it look ok now? I'd like to move this one forward if so.
There were two issues: * Not correctly detecting whether the action is pinned, when pinned to a major version branch, like "v7" in https://github.com/lukka/run-vcpkg. * Not correctly finding update candidates when pinned to a major version branch. In this case, the update candidates should include another major version branches, like "v10", but branches were being completely ignored as potential update candidates.
deivid-rodriguez
force-pushed
the
deivid-rodriguez/fix-bumping-actions-pinned-to-branches-that-look-like-tags
branch
from
9dd57ba to
181abaf
Compare
|
|
||
| # Otherwise, assume we're pinned | ||
| true | ||
| # TODO: Research whether considering branches that look like versions pinned makes sense for all ecosystems |
| @dependency = dependency | ||
| @credentials = credentials | ||
| @ignored_versions = ignored_versions | ||
| @raise_on_ignored = raise_on_ignored | ||
| @requirement_class = requirement_class | ||
| @version_class = version_class | ||
| @consider_version_branches_pinned = consider_version_branches_pinned |
There was a problem hiding this comment.
I like the approach of parameterising this rather than overriding it 🥇
|
Awesome, thanks so much @brrygrdn! |
deivid-rodriguez
deleted the
deivid-rodriguez/fix-bumping-actions-pinned-to-branches-that-look-like-tags
branch
If users have something like in the workflow file
And the given action, in this case
lukka/run-vcpkgkeeps the major version reference as a branch (as opposed to a tag), then Dependabot would not be able to create updates for that.This PR fixes the problem.
Closes #5017.