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

Lock down bundler to pre-2.2.10 and fix ManageIQ::Environment.bundler_version #21051

Merged
merged 1 commit into from
Feb 15, 2021

Conversation

agrare
Copy link
Member

@agrare agrare commented Feb 15, 2021

Bundler 2.2.10 has a bug causing bundle install to fail with gems being installed from one source that depend on gems from another. Until that is resolved we should lockdown bundler to max 2.2.9.

Also the ManageIQ::Environment.bundler_version regex isn't able to properly parse all gem requirements other than the first on. We can use the Bundler::Definition to parse the Gemfile "properly"

Alternative to: ManageIQ/manageiq-ui-classic#7634 (comment)

@agrare agrare force-pushed the lock_down_bundler_pre_2_2_10 branch from d91579a to 6369e44 Compare February 15, 2021 18:30
@@ -132,7 +132,12 @@ def self.update_ui

def self.bundler_version
gemfile = APP_ROOT.join("Gemfile")
File.read(gemfile).match(/gem\s+['"]bundler['"],\s+['"](.+?)['"]/)[1]
Copy link
Member Author

Choose a reason for hiding this comment

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

This would only return "~> 2.1"

lib/manageiq/environment.rb Outdated Show resolved Hide resolved
gemfile_dependencies = Bundler::Definition.build(gemfile, nil, {}).dependencies
bundler_dependency = gemfile_dependencies.detect { |dep| dep.name == "bundler" }

version_requirements = bundler_dependency.requirement.requirements
Copy link
Member Author

Choose a reason for hiding this comment

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

>> version_requirements
=> [["~>", #<Gem::Version "2.1">], [">=", #<Gem::Version "2.1.4">], ["<", #<Gem::Version "2.2.10">]]

@agrare agrare changed the title Lock down bundler to pre-2.2.10 and fix bundler_version Lock down bundler to pre-2.2.10 and fix ManageIQ::Environment.bundler_version Feb 15, 2021
@agrare agrare force-pushed the lock_down_bundler_pre_2_2_10 branch from 6369e44 to 4ce5246 Compare February 15, 2021 18:36
@agrare agrare force-pushed the lock_down_bundler_pre_2_2_10 branch from 4ce5246 to 71e5a0b Compare February 15, 2021 18:39
@agrare
Copy link
Member Author

agrare commented Feb 15, 2021

source $TRAVIS_BUILD_DIR/bin/ci/setup_ruby_env.sh
#!/bin/bash
./bin/ci/setup_ruby_environment.rb
Fetching bundler-2.2.9.gem
Successfully installed bundler-2.2.9
1 gem installed

@miq-bot
Copy link
Member

miq-bot commented Feb 15, 2021

Checked commit agrare@71e5a0b with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@agrare
Copy link
Member Author

agrare commented Feb 15, 2021

cc @Fryguy @NickLaMuro

@Fryguy
Copy link
Member

Fryguy commented Feb 15, 2021

LGTM, but does Bundler::Definition.build(gemfile, nil, {}).dependencies end up parsing the Gemfile? I'm concerned it will end up spitting out a 3rd and maybe even 4th copy of things like "override_gem"

@agrare
Copy link
Member Author

agrare commented Feb 15, 2021

Bundler::Definition.build(gemfile, nil, {}).dependencies does parse the Gemfile, that's a good question about override_gem I think it returns the "evaluated" gemfile so not one dependency per line-item but let me test that out.

@agrare
Copy link
Member Author

agrare commented Feb 15, 2021

Yeah so if I include e.g. override_gem "bundler", "=2.2.9" in bundler.d/*.rb then we only get the one dependency returned:

>> Bundler::Definition.build("Gemfile", nil, {}).dependencies.select { |dep| dep.name == "bundler" }
** override_gem("bundler", "=2.2.9") at /home/grare/adam/src/manageiq/manageiq/bundler.d/overrides.rb:9
=> [<Bundler::Dependency type=:runtime name="bundler" requirements="= 2.2.9">]

@NickLaMuro
Copy link
Member

NickLaMuro commented Feb 15, 2021

@agrare @Fryguy Cool, I didn't know about this script, and I do agree it is a better long term solution.

That said, it doesn't seem like the UI classic actually consumes this script like it does here in the base repo:

https://travis-ci.com/github/ManageIQ/manageiq-ui-classic/jobs/483230007#L254-L258

#### PULLED FROM THE TRAVIS OUTPUT

$ source bin/ci/before_install.sh
Gem 'bundler' is not installed
Fetching bundler-2.2.9.gem
Successfully installed bundler-2.2.9
1 gem installed

(bundler 2.2.9 here is only getting installed because that run is from the ManageIQ/manageiq-ui-classic#7634 build, and I am too lazy to find another)

So we might need to make some changes over in UI Classic regardless if we take this change or mine.

@agrare
Copy link
Member Author

agrare commented Feb 15, 2021

@NickLaMuro I believe the gem install bundler is done in bin/setup by ManageIQ::Environment.manageiq_plugin_setup(gem_root)

https://github.com/ManageIQ/manageiq-ui-classic/blob/master/bin/setup#L20

@agrare
Copy link
Member Author

agrare commented Feb 15, 2021

@miq-bot cross-repo-tests manageiq-ui-classic

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Feb 15, 2021
Copy link
Member

@NickLaMuro NickLaMuro left a comment

Choose a reason for hiding this comment

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

@agrare Good point. I clearly was missing that bit when I was double checking, and I think I agree that what you have here should work.

That said, I think in this particular case, the cross-repo tests, while they appear to be working as expected, aren't a perfect replication since we are simulating a travis run, but not replicating it. So there is a chance it doesn't work quite the same.

That said, I think you are still good, so 👍 from me.

@Fryguy Fryguy merged commit 7f28a62 into ManageIQ:master Feb 15, 2021
@agrare agrare deleted the lock_down_bundler_pre_2_2_10 branch February 15, 2021 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants