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

Bundler 2 [pre-release]: Add UpdateChecker #3349

Merged
merged 1 commit into from
Mar 25, 2021

Conversation

jurre
Copy link
Member

@jurre jurre commented Mar 25, 2021

Implements remaining native helpers to get the UpdateChecker to work on v2

@jurre jurre requested a review from a team as a code owner March 25, 2021 10:44
Copy link
Contributor

@brrygrdn brrygrdn left a comment

Choose a reason for hiding this comment

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

LGTM, I have only one question and it's something we can punt on as we have other things to do to settle the test suite structure, otherwise 🚀

allow(Dependabot::Bundler::NativeHelpers).
to receive(:run_bundler_subprocess).
with({
bundler_version: "2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this bundler version set in the stub the only difference between test version? WDYT think about just setting the expectation using something like:

bundler_version: bundler_2_available? ? "2" : "1"

We don't have the fixture swapping wired up based on SUITE_NAME but when we do, the gemfiles uses should be automagically pulled from the right folder, so it might save some test duplication.

Copy link
Member Author

Choose a reason for hiding this comment

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

The fixtures are also different between these tests, but I re-checked and it turns out they don't need to be, so let's go with your suggestion 💯

context "bundler v2", :bundler_v2_only do
let(:dependency_files) { project_dependency_files("bundler2/private_git_source") }

it "updates the dependency" do
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@@ -1,6 +1,6 @@
GEM
remote: https://rubygems.org/
remote: https://wxuokzLuQTRgMGtEYMPJ@repo.fury.io/greysteil/
remote: https://SECRET_CODES@repo.fury.io/greysteil/
Copy link
Contributor

Choose a reason for hiding this comment

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

😬 Thanks for ✂️ this

@jurre jurre force-pushed the jurre/bundler-v2-update-checker branch from 3bd282f to 9d2f19a Compare March 25, 2021 11:22
private_registry_versions: [:gemfile_name, :dependency_name, :dir, :credentials ],
jfrog_source: [:dir, :gemfile_name, :credentials, :using_bundler2],
git_specs: [:dir, :gemfile_name, :credentials, :using_bundler2],
jfrog_source: %i(dir gemfile_name credentials using_bundler2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this function isn't tested anywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

😬 Possible, we should have cleared out this file by now, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it is, I'll add some coverage 👍

Copy link
Contributor

@feelepxyz feelepxyz left a comment

Choose a reason for hiding this comment

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

🎉

@jurre jurre merged commit 0733f56 into main Mar 25, 2021
@jurre jurre deleted the jurre/bundler-v2-update-checker branch March 25, 2021 13:15
@thepwagner thepwagner mentioned this pull request Mar 25, 2021
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.

3 participants