Actions: Consider precision of current version when selecting latest #4953

mctofu · 2022-04-05T00:44:26Z

This attempts to address #4768

This has become noticeable recently with the release of the v3 versions of many github actions. The existing code to find the latest version did not deal with situations where multiple tags represented the latest version (v3, v3.0.0). Users that had pinned to a patch version were getting swapped to a major version (v2.1.5 -> v3) and some saw the opposite (v2 -> v3.0.0). Also, when v3.0.1 was released users pinned to a major version got updates to a patch version (v2 -> v3.0.1)

To work around this I've added a new method, GitCommitChecker.local_tags_for_latest_version_commit_sha, which returns all the version tags attached to the commit for the latest version. I then updated the Actions UpdateChecker to try and choose the version tag that matches the precision of the current version.

mctofu

I think the cargo failure is unrelated?

mctofu · 2022-04-05T01:21:16Z

github_actions/lib/dependabot/github_actions/update_checker.rb

+          latest_tags = git_commit_checker.local_tags_for_latest_version_commit_sha
+
+          # Find the latest version with the same precision as the pinned version.
+          # Falls back to a version with the closest precision if no exact match.


I wasn't sure what to do here but assumed we'd probably run into this somewhere in the wild. A simpler option would be to refuse to update if we couldn't find a match?

I think this approach is good, I think both outcomes are mildly surprising but failing over to some kind of update at least alerts the user to the new version and highlights that the tagging strategy on the library has changed.

Cases where we don't do anything are harder to relay to the end user without them reading the logs closely, so I think this is the better outcome.

Do we have a test that covers the tie break behaviour?

I haven't gone over the existing tests outside the diff, but it occurs to me we might already be testing this as a variation on the current behaviour but I couldn't spot it.

Nope, there were not. It's a bit of work to setup test fixtures for each case so I wanted to be sure we were OK with the approach first. I added some tests in aaa7703 by stubbing out the call to GitCommitChecker so the examples are easier to read.

mctofu · 2022-04-05T01:21:55Z

github_actions/spec/dependabot/github_actions/update_checker_spec.rb

@@ -133,7 +133,7 @@
        let(:reference) { "1c24df3" }

        let(:repo_url) { "https://api.github.com/repos/actions/setup-node" }
-        let(:comparison_url) { repo_url + "/compare/v1.1.0...1c24df3" }
+        let(:comparison_url) { repo_url + "/compare/v1.1...1c24df3" }


This was a side effect of adding a v1.1 version to the test fixture.

jurre · 2022-04-05T08:47:23Z

I think the cargo failure is unrelated?

Yeah they look related to git dependencies, but checked on a different branch and locally that the same errors exist, might be testing against some public deps that have changed

brrygrdn

I have a couple of questions, but overall this looks great 🥇

brrygrdn · 2022-04-05T11:00:42Z

common/lib/dependabot/git_commit_checker.rb

+      max_tag = tags.
+                max_by do |t|
+        version = t.name.match(VERSION_REGEX).named_captures.
+                  fetch("version")
+        version_class.new(version)
      end


This seems to be a duplication of lines 114-119, is it maybe worth extracting this to a method?

It took me a few minutes for the need to do the transform from the version back to the tags ( which are versions effectively and likely why I did a double-take 😅 ) on the commit sha to click, isolating this as a method might allow us to make it a little more expressive.

I tried to improve this in 9d1dafb but open to any other naming suggestions!

brrygrdn · 2022-04-05T11:04:40Z

github_actions/lib/dependabot/github_actions/update_checker.rb

+          latest_tags = git_commit_checker.local_tags_for_latest_version_commit_sha
+
+          # Find the latest version with the same precision as the pinned version.
+          # Falls back to a version with the closest precision if no exact match.


I think this approach is good, I think both outcomes are mildly surprising but failing over to some kind of update at least alerts the user to the new version and highlights that the tagging strategy on the library has changed.

Cases where we don't do anything are harder to relay to the end user without them reading the logs closely, so I think this is the better outcome.

brrygrdn · 2022-04-05T11:06:28Z

github_actions/spec/dependabot/github_actions/update_checker_spec.rb

+      context "using the full version" do
+        let(:reference) { "v1.0.0" }
+        it { is_expected.to eq(Dependabot::GithubActions::Version.new("2.1.3")) }
+      end


brrygrdn · 2022-04-05T11:10:35Z

github_actions/lib/dependabot/github_actions/update_checker.rb

+          latest_tags = git_commit_checker.local_tags_for_latest_version_commit_sha
+
+          # Find the latest version with the same precision as the pinned version.
+          # Falls back to a version with the closest precision if no exact match.


Do we have a test that covers the tie break behaviour?

I haven't gone over the existing tests outside the diff, but it occurs to me we might already be testing this as a variation on the current behaviour but I couldn't spot it.

jurre · 2022-04-05T14:42:10Z

I've fixed the cargo things

brrygrdn

🚀 Thanks for addressing those comments, this looks great!

pzygielo · 2022-04-11T14:30:02Z

Not sure if this is working as expected. I've just got PR "Bump actions/setup-java from 2 to 3.1.1"

updater | time="2022-04-11T14:14:27Z" level=info msg="starting job..." fetcher_timeout=5m0s job_id=339650753 updater_timeout=45m0s updater_version=0.180.5-fa5bb09ca507cf1caf68ca73cfd011f9620038d6

pzygielo · 2022-04-11T14:51:25Z

Not sure if this is working as expected. I've just got existing, open PR "Bump actions/setup-java from 2 to 3" superseded by "Bump actions/setup-java from 2 to 3.1.1"

updater | time="2022-04-11T14:31:40Z" level=info msg="starting job..." fetcher_timeout=5m0s job_id=339675945 updater_timeout=45m0s updater_version=0.180.5-fa5bb09ca507cf1caf68ca73cfd011f9620038d6

pzygielo · 2022-04-11T16:51:57Z

The #4953 (comment) and #4953 (comment) happened in the short time while setup-java 3 tag was still pointing at the previous release. Once this 3 was moved to the latest release new PRs consider precision again indeed.

I still do not think, that the case of major-only version not advanced in the same time as point-release should cause observed behaviour.

mctofu · 2022-04-11T17:44:33Z

Thanks @pzygielo, that's a good catch! A mismatch in the v3 and latest v3.x.x tag would definitely cause an issue when upgrading from a previous major version.

One argument for keeping the current behavior is that creating a PR for the v3 tag would not be updating you to the latest version. Could this cause problems for actions which don't follow the recommend practices for tagging?

An argument against the current behavior is it's inconsistent with the behavior for updates within a major version. If you are currently on the v3 tag which happens to be behind the latest v3.x.x tag you won't get an update. An update from v2 -> v3.x.x is only triggered as part of a major version upgrade.

I think it does make sense to try and preserve consistency and stick to upgrading from v2 -> v3, even when v3 isn't the latest release. We could do that by reworking the version selection to not require the v3 tag is from the same commit as the latest v3.x.x tag.

pzygielo · 2022-04-11T17:56:20Z

An update from v2 -> v3.x.x is only triggered as part of a major version upgrade.

That's what happened in my cases, indeed.

Should I accept the change "Bump actions/setup-java from 2 to 3.1.1" I'd be again in the loop of PRs for every bugfix release. (As x.x.x precision would be preferred then.)
While for some, let's say, more trustworthy actions, that are not project's dependencies but CI env only, I'd prefer to implicitly upgrade as the tag is or isn't moved by action vendor.

Thanks for checking.

mctofu added 5 commits April 4, 2022 16:26

Pin vcr cassette name

3c4e727

Pin vcr cassette name

8a43d8c

Pin vcr cassette name

13c0d13

Get all version tags applied to the latest version commit

5d804e2

Consider precision of current version when selecting latest

8b47346

mctofu commented Apr 5, 2022

View reviewed changes

mctofu marked this pull request as ready for review April 5, 2022 01:23

mctofu requested a review from a team as a code owner April 5, 2022 01:23

brrygrdn reviewed Apr 5, 2022

View reviewed changes

mctofu added 3 commits April 5, 2022 16:55

Extract method to get the max version from a set of tags

9d1dafb

Tests cases for precision mismatch

aaa7703

Merge branch 'main' into mctofu/actions-versions-stability

040e7f3

mctofu requested a review from brrygrdn April 6, 2022 00:48

Test when the commit is also tagged with a lower version

d8014ec

brrygrdn approved these changes Apr 6, 2022

View reviewed changes

mctofu mentioned this pull request Apr 6, 2022

Switch VCR record mode to :none #4822

Merged

mctofu merged commit 98098a4 into main Apr 6, 2022

mctofu deleted the mctofu/actions-versions-stability branch April 6, 2022 16:43

jurre mentioned this pull request Apr 6, 2022

v0.180.4 #4963

Merged

adamralph mentioned this pull request Apr 10, 2022

GitHub Actions - full patch versions are bumped to minimal new major versions #4768

Closed

mctofu mentioned this pull request Apr 11, 2022

dependabot not detecting latest version of github action #4834

Closed

exvuma mentioned this pull request Apr 11, 2022

Make dependabot respect granularity of previous semversioning #4779

Closed

nickmccurdy mentioned this pull request Jun 22, 2022

Allow bumping GH actions to rolling releases #2304

Closed

henryiii mentioned this pull request Jun 26, 2022

Add dependabot pypa/build#489

Merged

deivid-rodriguez mentioned this pull request Oct 16, 2022

Never change version precision of actions chosen by users #5891

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Actions: Consider precision of current version when selecting latest #4953

Actions: Consider precision of current version when selecting latest #4953

mctofu commented Apr 5, 2022 •

edited

Loading

mctofu left a comment

mctofu Apr 5, 2022

brrygrdn Apr 5, 2022 •

edited

Loading

brrygrdn Apr 5, 2022

mctofu Apr 6, 2022

mctofu Apr 5, 2022

jurre commented Apr 5, 2022

brrygrdn left a comment

brrygrdn Apr 5, 2022

mctofu Apr 6, 2022

brrygrdn Apr 5, 2022 •

edited

Loading

brrygrdn Apr 5, 2022

brrygrdn Apr 5, 2022

jurre commented Apr 5, 2022

brrygrdn left a comment

pzygielo commented Apr 11, 2022

pzygielo commented Apr 11, 2022

pzygielo commented Apr 11, 2022

mctofu commented Apr 11, 2022

pzygielo commented Apr 11, 2022

Actions: Consider precision of current version when selecting latest #4953

Actions: Consider precision of current version when selecting latest #4953

Conversation

mctofu commented Apr 5, 2022 • edited Loading

mctofu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brrygrdn Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jurre commented Apr 5, 2022

brrygrdn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brrygrdn Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jurre commented Apr 5, 2022

brrygrdn left a comment

Choose a reason for hiding this comment

pzygielo commented Apr 11, 2022

pzygielo commented Apr 11, 2022

pzygielo commented Apr 11, 2022

mctofu commented Apr 11, 2022

pzygielo commented Apr 11, 2022

mctofu commented Apr 5, 2022 •

edited

Loading

brrygrdn Apr 5, 2022 •

edited

Loading

brrygrdn Apr 5, 2022 •

edited

Loading