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: [Prerelease] Add native helpers for file parsing #3315

Merged
merged 5 commits into from
Mar 22, 2021

Conversation

feelepxyz
Copy link
Contributor

@feelepxyz feelepxyz commented Mar 22, 2021

Add native helpers for bundler 2 file parsing with testing setup to run bundler specs with both bundler 1 and 2.

The current setup runs all bundler tests twice, sequentially which will add around ~4 mins to the bundler test suite run time.

We could split and run these in parallel or make the bundler 2 tests optional somehow but this also complicates the test setup.

@feelepxyz feelepxyz marked this pull request as ready for review March 22, 2021 13:48
@feelepxyz feelepxyz requested a review from a team as a code owner March 22, 2021 13:48
@feelepxyz feelepxyz changed the title Add bundler 2 file parser Bundler 2: Add native helpers for file parsing Mar 22, 2021
@feelepxyz feelepxyz changed the title Bundler 2: Add native helpers for file parsing Bundler 2: [Prerelease] Add native helpers for file parsing Mar 22, 2021
@feelepxyz feelepxyz changed the title Bundler 2: [Prerelease] Add native helpers for file parsing Bundler 2: [Prerelease] Add native helpers for file parsing Mar 22, 2021
- python
- terraform
- { path: bundler, name: bundler1 }
- { path: bundler, name: bundler2 }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brrygrdn @jurre looks like you can add hashes as the suite argument, thoughts on doing something like this?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I dig it!

Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 Nice

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, the only downside of this approach is it seems to mess with our configured required builds, we'll need to flip the required builds from the old set to the new ones, merge this and then force all other PRs to merge master.

Not a big deal overall, as I think this is ultimately a better approach, just a one-time pain to switch over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Will fix that up 👍

@@ -20,4 +19,5 @@ cd "$install_dir"

# NOTE: Sets `BUNDLED WITH` to match the installed v1 version in Gemfile.lock
# forcing specs and native helpers to run with the same version
BUNDLER_VERSION=2 bundle install
BUNDLER_VERSION=2 bundle config set --local path ".bundle"
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

bundler/spec/spec_helper.rb Outdated Show resolved Hide resolved
@feelepxyz feelepxyz force-pushed the feelepxyz/bundler2-file-parser branch from c793e11 to 1975d59 Compare March 22, 2021 15:10
@feelepxyz feelepxyz merged commit 33037ec into main Mar 22, 2021
@feelepxyz feelepxyz deleted the feelepxyz/bundler2-file-parser branch March 22, 2021 15:24
@thepwagner thepwagner mentioned this pull request Mar 23, 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.

4 participants