-
Notifications
You must be signed in to change notification settings - Fork 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
Ensure Docker versions are valid Dependabot::Versions #7984
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.
Can we add a spec that reproduces this (and proves the fix?)
I added a couple of tests in f0a27a1 and fcbb08a.
If the test in fcbb08a isn't thorough enough I could add a new test fixture - I was able to recreated the issue in a public test repo. I know this patch works though because I can now run the previously erroring update locally:
|
967a99c
to
a94c248
Compare
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 looks good to me 👍
a94c248
to
e346ee5
Compare
Thank you for adding the tests, and sorry for the delayed response |
All good thanks for the initial review |
…-semver Ensure Docker versions are valid Dependabot::Versions
Fixes #7938
WIP
Dependabot::Docker::Version#new
would normally return an instance of aGem::Version
with an actual value, butDocker::Version#initialize
failed to call#super
and so it returnedGem::Version.new(nil)
instead.This PR fixes that issue in the
Dependabot::Docker::Version
class by actually calling#super
and it also removes the overloaded#self.correct?(version)
method that was present since the base method should now work correctly.Finally, in
Updater::GroupUpdateCreation
we make sure to cast the version returned by the latest version finder to an actualDependabot::Version
so we can use theDependabot::Version#segments
method.