Review multiple sources deprecation #7057
Conversation
|
This PR also includes some extra changes to refactor deprecation specs, and to move the tests for multiple sources support deprecation to the file that tests all deprecations. As always, I can move those to a separate PR if reviewers feel it's needed 👍. |
|
Ok, so this is failing because this introduces the lockfile incompatibility mentioned in #7007. I would love a review of that PR. |
deivid-rodriguez
force-pushed
the
multiple_sources_deprecation
branch
4 times, most recently
from
80f6904
to
a5c3549
Compare
I ended up extracting these changes to #7105, so I'm going to ask for reviews there first. |
deivid-rodriguez
force-pushed
the
multiple_sources_deprecation
branch
from
a5c3549
to
127bd96
Compare
`install_gemfile!` already runs that.
Gemfile is not considered in this situation.
Move them to the file where all deprecations are tested. And test just the simple case, since the deprecations logic should be the same for all cases.
Currenty one gets errors like ``` Failure/Error: lock_sources.to_set == replacement_sources.to_set NoMethodError: undefined method `to_set' for #<Array:0x0000560a01cd97c0> Did you mean? to_s ```
And fix specs.
deivid-rodriguez
force-pushed
the
multiple_sources_deprecation
branch
from
127bd96
to
a918651
Compare
|
This PR is now ready. I ended up keeping the instructions to opt-in to the fix in the deprecation message, but merging the Also, there was a failing spec in the list command specs, but it was not related to the list command, but to a slight change in behavior due to the multisource fix. I'm unsure whether we want this new behavior or not, but since it was unrelated to the list command and I don't want it to block this PR, I refactored the list command specs to not hit the behavior change, and documented the change separately on a different spec. See 59e989f. |
Previously, all specs would bundle a Gemfile from the "repo1" source by default. Then, some list specs would end up using a "repo2" source. The behavior when changing repo sources is changing with the `disable_multisource` setting, but it is unrelated to the list command. So, I refactored the list command specs to not change sources, and extracted the behavior change about changing sources to a separate spec.
deivid-rodriguez
force-pushed
the
multiple_sources_deprecation
branch
from
a918651
to
59e989f
Compare
|
@bundlerbot r+ |
Merge conflict (retrying...) |
7057: Review multiple sources deprecation r=indirect a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that I had not yet reviewed the deprecation about multiple global sources being present on a Gemfile, and thus the specs were skipped. ### What was your diagnosis of the problem? My diagnosis was that we need to delay the removal of multiple sources support to bundler 3, so that we can show the deprecations in the 2.x series. I also noticed that part of the deprecation message was inaccurate. In order to upgrade the warning to an error, you would also need to configure the `lockfile_uses_separate_rubygems_sources` setting. Otherwise you will get an error that these two settings depend on each other and can't be enabled separatedly. ### What is your fix for the problem, implemented in this PR? My fix is to delay these feature flags to bundler 3 so that the deprecation specs pass. Also, since before giving this advice I'd like to study why we have two different settings that can't be enabled separately, and why the can't be merged to a single one, I have removed that part of the message for now. ### Why did you choose this fix out of the possible options? I chose this fix because it keeps me moving with reviewing all the deprecations for bundler 3 breaking changes, and makes sure that the deprecations for this change of behavior are tested (and passing). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
Build succeeded |
ghost
merged commit 
What was the end-user problem that led to this PR?
The problem was that I had not yet reviewed the deprecation about multiple global sources being present on a Gemfile, and thus the specs were skipped.
What was your diagnosis of the problem?
My diagnosis was that we need to delay the removal of multiple sources support to bundler 3, so that we can show the deprecations in the 2.x series.
I also noticed that part of the deprecation message was inaccurate. In order to upgrade the warning to an error, you would also need to configure the
lockfile_uses_separate_rubygems_sourcessetting. Otherwise you will get an error that these two settings depend on each other and can't be enabled separatedly.What is your fix for the problem, implemented in this PR?
My fix is to delay these feature flags to bundler 3 so that the deprecation specs pass. Also, since before giving this advice I'd like to study why we have two different settings that can't be enabled separately, and why the can't be merged to a single one, I have removed that part of the message for now.
Why did you choose this fix out of the possible options?
I chose this fix because it keeps me moving with reviewing all the deprecations for bundler 3 breaking changes, and makes sure that the deprecations for this change of behavior are tested (and passing).