Skip to content

Consider precision when finding latest action#6102

Merged
deivid-rodriguez merged 4 commits intomainfrom
deivid-rodriguez/consider-precision-when-finding-latest-action
Nov 15, 2022
Merged

Consider precision when finding latest action#6102
deivid-rodriguez merged 4 commits intomainfrom
deivid-rodriguez/consider-precision-when-finding-latest-action

Conversation

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez commented Nov 10, 2022

If the major tag of an action does not point to the latest patch tag, Dependabot is not able to propose major updates. However, there's no reason why it should not propose an update to a higher major, even if it does not point to the latest patch level in that major series.

This is currently the case of the actions/cache action, and I think it was unintended, but I could imagine action authors doing this intentionally to get patch level versions tested in the while before they move the major tag.

This is stacked on top of #6052 and #6071 because it updates the same area of the code. Will unstack those later.

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/consider-precision-when-finding-latest-action branch 2 times, most recently from c40710d to 249cefe Compare November 14, 2022 13:01
@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review November 14, 2022 13:04
@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner November 14, 2022 13:04
Copy link
Copy Markdown
Member

@jurre jurre left a comment

Choose a reason for hiding this comment

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

This makes sense to me, thanks!

@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

For the record, I decided to unstack this one and get it ready before the one about security update support for Actions, because having this one in place should help us define the expected behavior for all edge cases when providing security updates for Actions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can raise an exception if the name doesn't match VERSION_REGEX. Would it make sense to use safe navigation here to return nil if the name doesn't match?

Suggested change
name.match(VERSION_REGEX).named_captures.fetch("version")
name.match(VERSION_REGEX)&.named_captures&.fetch("version")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking at the call sites, you may need to chain additional calls to compact to handle any returned nil values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is just an extraction of currently existing duplicated logic. I'm not sure if this can be actually fed names that don't match or if they are validated further up the stack.

My empirical approach would be that the check is not needed, since I don't think I have seen that error in the wild or reports pointing to it. But in any case seems unrelated to this PR, since the aggressive code is already there, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Further thinking about this, while I extracted existing instances of this logic to a method, I am indeed adding a new call site, so worth checking wether the case you mentioned can happen and add defensive code if yes. I will check this in more detail later 👍.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, all these helpers deal with "version tags" (the allowed_version_tags method specifically), which are essentially git tags that look like versions, i.e., have been filtered through this method:

https://github.com/dependabot/dependabot-core/blob/fbfca13459c89f0664807452ab207bfc2114f287/common/lib/dependabot/git_commit_checker.rb#L323-L325

On further iterations I will try to make this more clear, but I think this code is safe as is 👍.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mattt I'll merge this now based on my above comments, but if you still think there's something to address I'll open a followup PR!

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/consider-precision-when-finding-latest-action branch from 249cefe to fbfca13 Compare November 14, 2022 13:33
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/consider-precision-when-finding-latest-action branch from fbfca13 to 1c7d9ca Compare November 15, 2022 10:40
@deivid-rodriguez deivid-rodriguez merged commit 43034eb into main Nov 15, 2022
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/consider-precision-when-finding-latest-action branch November 15, 2022 12:21
@pavera pavera mentioned this pull request Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants