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

travis - run bundler with --no-deployment #1606

Merged
merged 1 commit into from
Jun 26, 2017
Merged

travis - run bundler with --no-deployment #1606

merged 1 commit into from
Jun 26, 2017

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Jun 26, 2017

According to the docs, travis runs bundler with --deployment when Gemfile.lock exists. It does, because bin/setup runs bundle install before that.

Which was fine until the combination of rm Gemfile.lock ; bundle install ; bundle install --deployment started failing (You are trying to install in deployment mode after changing your Gemfile...) :).

I don't think we want --deployment since we're gitignoring Gemfile.lock.

This fixes travis in the sense that it gets to running the specs. Not quite enough to make it green, but I'd rather keep the PRs separate.

to prevent breakage coming from inconsistencies in Gemfile.lock
@miq-bot miq-bot added the wip label Jun 26, 2017
@miq-bot
Copy link
Member

miq-bot commented Jun 26, 2017

Checked commit https://github.com/himdel/manageiq-ui-classic/commit/82e8db494001f63c0e3613a670d9ca17b49e5689 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 🍪

@himdel himdel changed the title [WIP] travis - run bundler with --no-deployment travis - run bundler with --no-deployment Jun 26, 2017
@himdel himdel added test and removed wip labels Jun 26, 2017
@h-kataria
Copy link
Contributor

Merging, travis failures are not related

@h-kataria h-kataria added this to the Sprint 64 Ending Jul 3, 2017 milestone Jun 26, 2017
@h-kataria h-kataria merged commit eedbe18 into ManageIQ:master Jun 26, 2017
@himdel himdel deleted the travis-deploy branch June 26, 2017 16:35
@himdel
Copy link
Contributor Author

himdel commented Jun 26, 2017

More info.. The failure got exposed by ManageIQ/manageiq#14525, but would happen any time there's a relative path to gems involved.. Since the first bundle install runs in spec/manageiq/ but the second in ui-classic, relative paths are not the same, but get saved as relative in Gemfile.lock (even when absolute in Gemfile).

So.. --no-deployment is probably the right fix :).

@Fryguy
Copy link
Member

Fryguy commented Jun 26, 2017

Why no-deployment over just fully qualifying the path? no-deployment just seems very unclear from an outside-in perspective. cc @jrafanie

@NickLaMuro
Copy link
Member

NickLaMuro commented Jun 26, 2017

@Fryguy Part of the explanation can be found in the bundler ticket I opened up:

rubygems/bundler#5817

The other end of the story is that path gems store the path as a relative path, to avoid something like /home/nicklamuro/path/to/my/workspace/main_project/vendor/my_vendored_gem sitting in a source controlled lockfile (making an assumption here after I basically asked the same question, but I think this makes sense). I tried updating the :path to something like File.expand_path("../mime-types-redirector", __FILE__), but that didn't help.

@himdel
Copy link
Contributor Author

himdel commented Jun 27, 2017

Also, it seems like --deployment is really just for projects which use Gemfile.lock in git. Am I wrong?

@jrafanie
Copy link
Member

@himdel I guess I don't understand why we'd have Gemfile.lock if it's .gitignored. I don't see Gemfile.lock in git. I don't understand.

@NickLaMuro
Copy link
Member

@jrafanie bundle install gets run on Travis twice in a build (once for the bin/setup step, and again by the default bundler route), and the Gemfile.lock is generated after the bin/setup, which allows --deployment to function.

@NickLaMuro
Copy link
Member

Whether or not the second bundle install is excessive or not another item entirely, but pretty sure it is being done in most provider repos.

@himdel
Copy link
Contributor Author

himdel commented Jun 27, 2017

👍 .. also, bin/setup gets run in before_install, so the second bundle install is travis' install step.

I'm thinking if we actually need that second bundle install for anything.. maybe we may want to drop it altogether (I think you can override the install command to just do echo or something (well, or bin/setup maybe :))).

But I'm not entirely sure... you definitely need a bundle install in ui-classic's dir to run individual specs by hand .. I only think that you may not need it when those are run by rake.

@NickLaMuro
Copy link
Member

NickLaMuro commented Jun 27, 2017

But I'm not entirely sure... you definitely need a bundle install in ui-classic's dir to run individual specs by hand .. I only think that you may not need it when those are run by rake.

Just in case when I mispoke yesterday in gitter confused anyone, when you run ManageIQ::Environment.manageiq_plugin_setup (called from bin/setup in this repo), the bundle install step from that is in fact run from the plugin's root directory, and not the nested manageiq directory.

So yea, @himdel is probably right and we should be able to remove travis' bundle install step, but we might have to do this in all of the plugin repos. Unsure.

@Fryguy
Copy link
Member

Fryguy commented Jun 30, 2017

I'm thinking if we actually need that second bundle install for anything.. maybe we may want to drop it altogether

Before the code was extracted to bin/setup (in ManageIQ/manageiq), we had overridden install. So, since it moved out, then yes, it should be dropped...it should not bundle install twice.

but we might have to do this in all of the plugin repos. Unsure.

that's fine

@Fryguy
Copy link
Member

Fryguy commented Jun 30, 2017

Interestingly enough, ManageIQ/manageiq itself doesn't use bin/setup in the .travis.yml. I'm debating whether it should or shouldn't

Fryguy added a commit to Fryguy/manageiq that referenced this pull request Nov 6, 2017
Relative pathing can have problems if the pwd is not the manageiq
directory.  This ensures the correct path.

Related:
- ManageIQ#14525
- ManageIQ/manageiq-ui-classic#1606
- ManageIQ/manageiq-content#136
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants