Skip to content
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

go_modules: Gracefully handle +incompatible versions when checking for updates #3661

Merged
merged 8 commits into from
May 7, 2021

Conversation

jasonrudolph
Copy link
Contributor

@jasonrudolph jasonrudolph commented May 6, 2021

This pull request aspires to resolve a regression introduced in #3631: When the latest version of a dependency is an +incompatible version, the update fails with an exception.

Behavior before and after #3631

Given the following go.mod file:

module github.com/foo/bar

go 1.16

require github.com/go-yaml/yaml v2.0.0+incompatible

Prior to #3631, the dry-run script ran as follows:

$ bin/dry-run.rb go_modules foo/bar

=> cloning into /tmp/d20210506-15-1uf8iat
=> parsing dependency files
=> updating 1 dependencies: github.com/go-yaml/yaml

=== github.com/go-yaml/yaml (2.0.0+incompatible)
 => checking for updates 1/1
 => latest available version is 2.0.0+incompatible
 => latest allowed version is 2.0.0+incompatible
    (no update needed as it's already up-to-date)

After #3631, the dry-run script errors as follows:

$ bin/dry-run.rb go_modules foo/bar

=> cloning into /tmp/d20210506-3909-h1fcc1
=> parsing dependency files
=> updating 1 dependencies: github.com/go-yaml/yaml

=== github.com/go-yaml/yaml (2.0.0+incompatible)
 => checking for updates 1/1
Traceback (most recent call last):
	15: from bin/dry-run.rb:601:in `<main>'
	14: from bin/dry-run.rb:601:in `each'
	13: from bin/dry-run.rb:609:in `block in <main>'
	12: from /home/dependabot/dependabot-core/go_modules/lib/dependabot/go_modules/update_checker.rb:41:in `latest_version'
	11: from /home/dependabot/dependabot-core/go_modules/lib/dependabot/go_modules/update_checker.rb:34:in `latest_resolvable_version'
	10: from /home/dependabot/dependabot-core/go_modules/lib/dependabot/go_modules/update_checker/latest_version_finder.rb:34:in `latest_version'
	 9: from /home/dependabot/dependabot-core/go_modules/lib/dependabot/go_modules/update_checker/latest_version_finder.rb:44:in `fetch_latest_version'
	 8: from /home/dependabot/dependabot-core/go_modules/lib/dependabot/go_modules/update_checker/latest_version_finder.rb:53:in `available_versions'
	 7: from /home/dependabot/dependabot-core/common/lib/dependabot/shared_helpers.rb:42:in `in_a_temporary_directory'
	 6: from /usr/lib/ruby/2.6.0/tmpdir.rb:93:in `mktmpdir'
	 5: from /home/dependabot/dependabot-core/common/lib/dependabot/shared_helpers.rb:45:in `block in in_a_temporary_directory'
	 4: from /home/dependabot/dependabot-core/common/lib/dependabot/shared_helpers.rb:45:in `chdir'
	 3: from /home/dependabot/dependabot-core/common/lib/dependabot/shared_helpers.rb:45:in `block (2 levels) in in_a_temporary_directory'
	 2: from /home/dependabot/dependabot-core/go_modules/lib/dependabot/go_modules/update_checker/latest_version_finder.rb:54:in `block in available_versions'
	 1: from /home/dependabot/dependabot-core/common/lib/dependabot/shared_helpers.rb:159:in `with_git_configured'
/home/dependabot/dependabot-core/go_modules/lib/dependabot/go_modules/update_checker/latest_version_finder.rb:73:in `block (2 levels) in available_versions': private method `select' called for nil:NilClass (NoMethodError)

About the proposed fix

Prior to #3631, if the updatechecker helper was unable to find a version that could be upgraded to, it always returned the current version of the dependency [code]. With the changes in #3631, the updatechecker helper instead returns nil in this scenario.

With the changes in this pull request, we now check for a nil return value from the updatechecker helper, and we return the current version of the dependency [code]. As a result, the calling function (Dependabot::GoModules::UpdateChecker#latest_resolvable_version) handles this scenario the same as it did prior to #3631. To confirm that, here's the behavior of the dry-run script following the changes in this pull request:

$ bin/dry-run.rb go_modules foo/bar

=> cloning into /tmp/d20210506-15-1uf8iat
=> parsing dependency files
=> updating 1 dependencies: github.com/go-yaml/yaml

=== github.com/go-yaml/yaml (2.0.0+incompatible)
 => checking for updates 1/1
 => latest available version is 2.0.0+incompatible
 => latest allowed version is 2.0.0+incompatible
    (no update needed as it's already up-to-date)

Fixes: #3664

@jasonrudolph jasonrudolph force-pushed the gracefully-handle-incompatible-gomod-versions branch from f64e88d to 7ec451f Compare May 6, 2021 20:22
Prior to this change, if there were no available versions, GetVersions
returned nil. Now, GetVersions will instead return an empty array in
this scenario.
@jasonrudolph jasonrudolph force-pushed the gracefully-handle-incompatible-gomod-versions branch from 7ec451f to e0532f8 Compare May 6, 2021 20:25
@jasonrudolph jasonrudolph requested a review from mctofu May 6, 2021 21:47
@jasonrudolph jasonrudolph marked this pull request as ready for review May 6, 2021 22:12
@jasonrudolph jasonrudolph requested a review from a team as a code owner May 6, 2021 22:12
Copy link
Contributor

@mctofu mctofu left a comment

Choose a reason for hiding this comment

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

Fix looks good!

require "dependabot/go_modules/update_checker/latest_version_finder"

RSpec.describe Dependabot::GoModules::UpdateChecker::LatestVersionFinder do
let(:dependency_name) { "github.com/dependabot-fixtures/go-modules-lib" }
Copy link
Contributor

Choose a reason for hiding this comment

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

For context: we are planning to move the #latest_resolvable_version tests from

to here so it makes sense to have this as the default even though it's not used.

Copy link
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.

Fix seems good! I wonder if we should log when this happens?

@jasonrudolph
Copy link
Contributor Author

jasonrudolph commented May 7, 2021

I wonder if we should log when this happens?

@jurre: Can you say more about the kind of logging you have in mind?

I'll probably ship this as-is to fix things for the folks that are affected by #3664, but I'm happy to circle back afterwards and add some logging if that seems appropriate.

@jasonrudolph jasonrudolph merged commit eb4f477 into main May 7, 2021
@jasonrudolph jasonrudolph deleted the gracefully-handle-incompatible-gomod-versions branch May 7, 2021 16:45
@mctofu
Copy link
Contributor

mctofu commented May 7, 2021

I think it's worth revisiting later to see if we should change the behavior in this case from "No updated needed" to "Update not possible".

It seems like the cause of this was when you were using an incompatible version but other compatible versions exist. Dependabot is unable to update you to a newer version in that case so "Update not possible" would be a better message to signal that a manual intervention is needed. The user would need to manually update to a compatible version to get updates back on track.

@jeffwidman
Copy link
Member

Dependabot is unable to update you to a newer version

For clarification, why is this? Is it because Dependabot can't figure out the newer version, or because it knows a newer version exists but isn't sure how to apply it?

From the PR description I assume the former (in which case logging an "Unable to determine update" might be appropriate), but from @mctofu 's comment above, it seems he thinks it might be the latter. If the latter, I wonder if #3590 might fix it.... don't know, just thinking aloud.

@mctofu
Copy link
Contributor

mctofu commented May 7, 2021

For clarification, why is this? Is it because Dependabot can't figure out the newer version, or because it knows a newer version exists but isn't sure how to apply it?

It's because Dependabot can't figure out the newer version. We traced this to the getVersions helper not getting any results when querying for versions here:

versions, err := repo.Versions("")

That is different behavior than if we were using go list to get the versions:

% go list -m -versions github.com/go-yaml/yaml
github.com/go-yaml/yaml v2.0.0+incompatible v2.1.0+incompatible

That doesn't give any indication that a compatible version exists since it would be a major version upgrade but with those results Dependabot would be able to upgrade to a new incompatible version if available.

@jeffwidman
Copy link
Member

Makes sense, thanks!

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.

New unhandled error in go_modules update_checker code
4 participants