-
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
Update lower boundary in Python pip pyproject when bumping versions #6631 #10060
Update lower boundary in Python pip pyproject when bumping versions #6631 #10060
Conversation
Smoke tests are updated: https://github.com/dependabot/smoke-tests/pull/212/files |
24ed6c0
to
f304dc8
Compare
Tagging @jeffwidman and @deivid-rodriguez for PR reviews too |
@kbukum1 The python CI test is failing with your last commit, so I have revoked the approval. |
Fixing it |
91394ad
to
f304dc8
Compare
ead807d
to
bb91a7e
Compare
fixed |
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.
Hei! I did not review the implementation, but there's some things that could be improved in my opinion in the new test outcomes.
|
||
context "when requirement version has more digits than the new version" do | ||
let(:requirement_txt_req_string) { "<=1.9.2,>=1.9" } | ||
let(:latest_resolvable_version) { "1.10" } | ||
|
||
its([:requirement]) { is_expected.to eq(">=1.9,<=1.10") } | ||
its([:requirement]) { is_expected.to eq("<=1.10,>=1.10") } |
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.
In this case, there's no clear solution, but <=1.10,>=1.10
feels not right? Either one of:
<=1.11,>=1.10
.<=1.10.2,>=1.10
- :unfixable
I'd probably leave this one alone as :unfixable
since it's unclear what the user wants in the case.
@@ -370,7 +388,7 @@ | |||
context "when the requirement version is too high" do | |||
let(:setup_cfg_req_string) { ">=2.0.0" } | |||
|
|||
its([:requirement]) { is_expected.to eq(:unfixable) } | |||
its([:requirement]) { is_expected.to eq(">=1.5.0") } |
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 like the previous outcome better here. If the latest resolvable version is 1.5.0, and user has specified >=2.0.0
in their manifest, I think either:
- User setup/manifest is wrong.
- Dependabot run into a bug when figuring out the latest resolvable version.
Either way, it seems best to leave this kind of ill defined case alone?
Also, the new solution is actually widening the requirement, not increasing it, which seems completely against the spirit of this PR?
@@ -381,7 +399,7 @@ | |||
context "when needing an update" do | |||
let(:setup_cfg_req_string) { ">=1.3.0, <1.5" } | |||
|
|||
its([:requirement]) { is_expected.to eq(">=1.3.0,<1.6") } | |||
its([:requirement]) { is_expected.to eq(">=1.5.0,<1.6") } |
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.
Same as the previous comment on the similar case on requirement.txt
files.
|
||
its([:requirement]) { is_expected.to eq(:unfixable) } | ||
end | ||
end |
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.
The above block of specs makes sense to me 👍.
@@ -129,7 +147,7 @@ | |||
context "when requirement version is too high" do | |||
let(:requirement_txt_req_string) { ">=2.0.0" } | |||
|
|||
its([:requirement]) { is_expected.to eq(:unfixable) } | |||
its([:requirement]) { is_expected.to eq(">=1.5.0") } |
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 like the previous outcome better here. If the latest resolvable version is 1.5.0, and user has specified >=2.0.0 in their manifest, I think either:
- User setup/manifest is wrong.
- Dependabot run into a bug when figuring out the latest resolvable version.
Either way, it seems best to leave this kind of ill defined case alone?
Also, the new solution is actually widening the requirement, not increasing it, which seems completely against the spirit of this PR?
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.
The new solution actually it is not widening it. It is setting the lower bound as the latest version and the upper bound is still following the last version by increasing. As it is mentioned in the document tried to follow the typical scenario.
Always increase the minimum version requirement to match the new version. If a range already exists, typically this only increases the lower bound.
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.
If I'm understanding this test case correctly, the previous requirement is >= 2.0.0
, correct? If Dependabot sets the new range to be >= 1.5.0
, it's actually decreasing the lower bound and thus widening the requirement.
This seems like an ill-defined case to me. Why would the existing range be >= 2.0.0
if the latest version is 1.5.0? That's why it seemed reasonable to not mess with this case and call it "unfixable".
That said, I just came up with a potential case when this could happen in real life: If latest version 2.0.0 is yanked, then that could make 1.5.0 the "new" latest version and make a case like this possible.
Previously Dependabot would avoid messing up with this case, and now it's downgrading the version to 1.5.0. I guess it's not as bad after all!
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 am strongly of the opinion that dependabot should never lower a >=
constraint. When such a constraint is used, I believe it means one of two things:
- We're tracking the latest version: the user should pick the
increase
strategy. - We're declaring the range of all compatible versions and we know that lower versions are not compatible (e.g. we're using a feature that was introduced in 2.0): user should pick
widen
strategy and dependabot should not touch lower bound.
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 realize I got the context wrong: we're talking about increase
here. I still think a user intervention would be more appropriate here, but I am not as strongly invested as for the widen case.
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.
Yeah, I do think there could be all sort of expectations here. I guess it could also happen that someone using widen
has changed their code to support 2.0 version of some library and bumped their lower bound requirement to >=2.0
, then 2.0 is abruptly removed and user may appreciated that Dependabot "rolls back" their requirement change and let them know through a PR they need to revert code changes too to get back on track.
Also, strictly speaking, for the widen
strategy, Dependabot is also doing exactly what's advertised right? Namely, "widen the range to accommodate for the latest version". Just that in this case, the latest version is not newer than the upper bound, but older than the lower bound.
In any case, I think this is exactly why I advocate for "leave this kind of case alone", because different users will have different expectations, or even the same user may want different things depending on the library.
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.
You do make a good point, this is a case where it's difficult to find a universal approach to fit everyone's use case. Your suggestion to leave it alone sounds sensible to me (biased though I am because that is what I've been advocating regardless).
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.
When I implemented this, my thoughts were as follows:
In real-world scenarios, this occurs when no range was specified before, when a higher version is yanked and necessitates a downgrade to a previous release, or when the strategy wasn’t set to increase when a lower bound is added. In such cases, Dependabot can adjust the lower bound accordingly.
For the increase strategy, if a lower constraint is defined to ensure compatibility, the lower bound should not be higher than the latest version since the latest version is functional for the app or library. Otherwise, it risks excluding versions that work correctly, potentially breaking the app.
I would appreciate your thoughts on this.
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 see what you're saying, but I still feel like it's a risky assumption to make for the widen strategy. For example, consider the case where a library detects a breaking bug in their latest release, so they yank it and then release a patched one soon after. If they do it in that order, dependabot might run in the window where the broken one was yanked but the new one not yet released. It would then change the lower bound, while it wouldn't have taken action at all if it had run a few minutes later.
If the library does not release a patched version (which is quite rare as far as I know), then action will indeed be required. But since this is such a rare occurrence, and lowering the constraint might not always be desired, I feel like it's safer to leave this as a problem for the user to solve.
That said, I'm just a bystander here, so I won't try to push my view any further than this.
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 agree with @sanderr here. I think Dependabot should create update PRs that work for everyone, or when that's not the case, at least allow the behavior to be configurable. This seems such an edge case that feels best to not try be too smart and leave it for the user to fix manually.
@@ -504,7 +552,7 @@ | |||
context "when the requirement version is too high" do | |||
let(:pyproject_req_string) { ">=2.0.0" } | |||
|
|||
its([:requirement]) { is_expected.to eq(:unfixable) } | |||
its([:requirement]) { is_expected.to eq(">=1.5.0") } |
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.
Same as the previous comment on the similar case on requirement.txt
files.
@@ -521,7 +569,7 @@ | |||
context "when needing an update" do | |||
let(:pyproject_req_string) { ">=1.3.0, <1.5" } | |||
|
|||
its([:requirement]) { is_expected.to eq(">=1.3.0,<1.6") } | |||
its([:requirement]) { is_expected.to eq(">=1.5.0,<1.6") } |
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.
Same as the previous comment on the similar case on requirement.txt
files.
|
||
its([:requirement]) { is_expected.to eq(:unfixable) } | ||
end | ||
end |
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.
The above block of specs makes sense to me 👍.
@@ -650,7 +728,7 @@ | |||
context "when the requirement is too high" do | |||
let(:pyproject_req_string) { ">=2.0.0" } | |||
|
|||
its([:requirement]) { is_expected.to eq(:unfixable) } | |||
its([:requirement]) { is_expected.to eq(">=1.5.0") } |
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.
Same as the previous comment on the similar case on requirement.txt
files.
@@ -661,7 +739,7 @@ | |||
context "when needing an update" do | |||
let(:pyproject_req_string) { ">=1.3.0, <1.5" } | |||
|
|||
its([:requirement]) { is_expected.to eq(">=1.3.0,<1.6") } | |||
its([:requirement]) { is_expected.to eq(">=1.5.0,<1.6") } |
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.
Same as the previous comment on the similar case on requirement.txt
files.
Also, I think it's fine to treat this a bug and change how Dependabot updates manifests here. However, as pointed out by @sanderr here, it'd be nice to enable an explicit |
Thanks @deivid-rodriguez! I was going to take a look at this--just running late--but @kbukum1 I'll wait until you put up another follow-on PR addressing some of this feedback and then take a deeper look myself. |
I am currently preparing a doc for the current increase and widening versioning strategies. After going through reviews here I plan to create a new issue that is going to be used for finalizing both versioning strategies. Is that sound ok @deivid-rodriguez , @jeffwidman |
The changes of this PR are reverted in #10194. The versioning strategy needs to be re-planned, and a solution will be implemented according to the new plan. |
What are you trying to accomplish?
The goal of this PR is to correct the behavior of the versioning strategy in Dependabot for Python dependencies. Specifically, when using the "increase" strategy.
Why: The current behavior was widening the range (e.g., from
>=8,<9
to>=8,<10
), which is not the intended behavior. Instead, it should keep lower bound version as the new latest version and just move forward the upper boundary version (e.g., from>=8,<9
to>=9,<10
) as mentioned Documentation for Versioning StrategiesIssues this affects or fixes:
increase
strategy should move ranges forward, not widen them #6631.Anything you want to highlight for special attention from reviewers?
The changes involve modifying the requirements for the pip ecosystem to ensure correct behavior for the "increase" strategy. Although the issue suggests above major versioning (e.g. >=8,<9), our "increase" strategy mandates that the lower bound is always to be the latest version (e.g. >=8.0.1,<9). This approach aligns with the guidelines provided in the Dependabot documentation. Therefore, the changes have been made accordingly.
How will you know you've accomplished your goal?
Update Smoke Tests for "Increase" Versioning Strategy Adjustment in Python Dependencies smoke-tests#212
Checklist