-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
go_modules: Gracefully handle +incompatible versions when checking for updates #3661
Conversation
f64e88d
to
7ec451f
Compare
Prior to this change, if there were no available versions, GetVersions returned nil. Now, GetVersions will instead return an empty array in this scenario.
7ec451f
to
e0532f8
Compare
This reverts commit 11130fa. We're gonna use a different approach. Stay tuned!
go_modules/spec/dependabot/go_modules/update_checker/latest_version_finder_spec.rb
Outdated
Show resolved
Hide resolved
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.
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" } |
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.
For context: we are planning to move the #latest_resolvable_version
tests from
describe "#latest_resolvable_version" do |
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.
Fix seems good! I wonder if we should log when this happens?
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. |
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. |
It's because Dependabot can't figure out the newer version. We traced this to the
That is different behavior than if we were using
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. |
Makes sense, thanks! |
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:
Prior to #3631, the dry-run script ran as follows:
After #3631, the dry-run script errors as follows:
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, theupdatechecker
helper instead returnsnil
in this scenario.With the changes in this pull request, we now check for a
nil
return value from theupdatechecker
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:Fixes: #3664