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

Add an options argument to the bundler_version helper method that is passed from the public API down #3230

Merged
merged 17 commits into from
Mar 17, 2021

Conversation

brrygrdn
Copy link
Contributor

@brrygrdn brrygrdn commented Mar 4, 2021

In order to start testing bundler 2 support, we want to be able to optionally use it in Dependabot core by passing an options[:bundler_2_available] flag into the Fetcher, Parser and Updater APIs from the running environment.

This PR adds this configuration layer which currently results in the Bundler native helper using a stubbed V2 library which returns a NotImplemented error for the existing method signatures.

This error is this used as a testing proof that our top-level classes fail due to this error if we pass in the options to verify we have passed the configuration correctly. This is further verified by making options a required argument of the bundler_version helper method, ensuring we get an ArgumentError in any cases we've missed.

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.

Nice one, are there other places left for this or should we get this in?

@brrygrdn
Copy link
Contributor Author

brrygrdn commented Mar 8, 2021

I think there's a few more, my plan was to take this PR right up to having a null adapter that just raises if the feature is on

@feelepxyz
Copy link
Contributor

I think there's a few more, my plan was to take this PR right up to having a null adapter that just raises if the feature is on

Ah yes that makes sense 👌

@brrygrdn brrygrdn force-pushed the brrygrdn/add-experimental-flag-for-bundler-2 branch 2 times, most recently from a19f047 to 9440f33 Compare March 15, 2021 12:52
bundler/helpers/v2/build Outdated Show resolved Hide resolved
@brrygrdn brrygrdn force-pushed the brrygrdn/add-experimental-flag-for-bundler-2 branch from 34dc277 to fa7ed79 Compare March 15, 2021 15:02

def bundler_version
@bundler_version ||= Helpers.bundler_version(lockfile)
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tentatively removed this as I think all the classes mixing it in are overriding it locally, but CI may prove me wrong

@brrygrdn brrygrdn changed the title [WIP] Add an options argument into more base classes to allow us to pass feature flags for bundler 2 Add an options argument to the bundle_version helper method that is passed from the public API down Mar 15, 2021
@brrygrdn brrygrdn changed the title Add an options argument to the bundle_version helper method that is passed from the public API down Add an options argument to the bundler_version helper method that is passed from the public API down Mar 15, 2021
@brrygrdn brrygrdn marked this pull request as ready for review March 15, 2021 16:19
@brrygrdn brrygrdn requested a review from a team as a code owner March 15, 2021 16:19
require "json"

$LOAD_PATH.unshift(File.expand_path("./lib", __dir__))
$LOAD_PATH.unshift(File.expand_path("../v1/monkey_patches", __dir__))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$LOAD_PATH.unshift(File.expand_path("../v1/monkey_patches", __dir__))
$LOAD_PATH.unshift(File.expand_path("../v2/monkey_patches", __dir__))

Copy link
Contributor

@feelepxyz feelepxyz Mar 15, 2021

Choose a reason for hiding this comment

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

oh wait this is intentional, ignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I flip flopped on that, I was nervous about hard forking monkey patches. It might make sense to go ahead and fork them but for now I just wanted to avoid breaking the loading.

@brrygrdn brrygrdn force-pushed the brrygrdn/add-experimental-flag-for-bundler-2 branch from e97f584 to 49f90f7 Compare March 15, 2021 16:27
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.

Thanks for testing this! LGTM 💯

@feelepxyz
Copy link
Contributor

Looks like quite a few classes require options to be passed in

@brrygrdn
Copy link
Contributor Author

Aha, I made it a required argument and didn't update the unit tests 🤕 Should be a relatively quick fix.

@brrygrdn brrygrdn force-pushed the brrygrdn/add-experimental-flag-for-bundler-2 branch from 793a97d to ef56726 Compare March 15, 2021 17:38
@brrygrdn
Copy link
Contributor Author

The CI failures seem to be limited to the new tests that use the v2 native helper, but I'm not seeing them locally, I suspect the issue is the v2 helper just isn't set up properly for CI yet. I've committed DEBUG_HELPERS=true to verify this while I go through the dockerfile and ci workflow.

@brrygrdn brrygrdn force-pushed the brrygrdn/add-experimental-flag-for-bundler-2 branch from f5af517 to ed20d2f Compare March 16, 2021 10:36
@brrygrdn
Copy link
Contributor Author

Yep, that was the issue, I forgot to add v2 to the Dockerfile and had some build script tweaks to make 😅

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.

🙌 just did a release of core so could merge this in now and release after?

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.

2 participants