-
Notifications
You must be signed in to change notification settings - Fork 950
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
Actions: improve semver support #3662
Conversation
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.
Just a checkpoint as it's EOD for me, feedback welcome!
return false unless ref.match?(/^[0-9a-f]{6,40}$/) | ||
return false unless ref&.match?(/^[0-9a-f]{6,40}$/) | ||
|
||
return false unless pinned? |
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.
This is performance: pinned?
"does things" - but a regexp check is local and free!
The previous commit updated the vendored `git-upload-pack` response, which redefined "up to date" for this test that uses the same fixture.
bcf3640
to
18217bb
Compare
A neat side effect of this change, Actions pinned to a commit SHA get a more clear PR title:
|
@@ -4,6 +4,21 @@ | |||
|
|||
module DummyPackageManager | |||
class Version < Gem::Version | |||
def initialize(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.
Why is this the same as Dependabot::GithubActions::Version
now?
In the tests for local_tag_for_pinned_version
, I'm using a git-upload-pack
captured from actions/cache
https://github.com/actions/checkout/tags
Having the active version_class
(which in that test - is DummyPackageManager::Version
) understand those tags was the path of least resistance.
Alternatively I could setup and capture a cleaner fixture - I liked the idea of the test case being "real".
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'm not very familiar with this updater but the changes make sense to me!
Since we strip the "v" when populating the dependency version the relative ignore conditions should also start working without any other changes?
github_actions/spec/dependabot/github_actions/update_checker_spec.rb
Outdated
Show resolved
Hide resolved
Yep, exactly!
|
@thepwagner @mctofu Could this PR have caused #3704? |
This improves the handling of GitHub Actions workflow files when the Action is fetched from a GitHub repository using semver-compatible tags, as suggested by Actions' best practices.
Our goal is to increase the likelihood that
Depedendency#version
in thegithub_actions
package manager is a valid semver tag. That is what allows the update types feature to derive relative versions.Changes focus on the
FileParser
:uses: actions/checkout@v2.3.3
, consider that theDependency.version == "2.3.3"
uses: actions/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675
, AND that commit corresponds to a valid semver tag, consider the tag to beDependency.version == "2.3.3"
.Preferring full length commit SHA is another GitHub best practice that Dependabot should expect and encourage.
Pinned to a sha
Pre:
Post:
Pinned to version
Pre:
Post: