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

Fix bundle override in ManageIQ::Environment #15585

Merged
merged 1 commit into from
Jul 17, 2017

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Jul 17, 2017

Commit 04d9a75 overrides Travis' built-in bundle install, which handles
bundle caching as well as parallel installs and retries, but did so
without any of those feature causing them to be lost in all of the
provider repos.

http://talk.manageiq.org/t/can-we-speed-up-small-repos-on-travis/2533

@bdunne Please review.
cc @durandom @cben

Commit 04d9a75 overrides Travis' built-in bundle install, which handles
bundle caching as well as parallel installs and retries, but did so
without any of those feature causing them to be lost in all of the
provider repos.

http://talk.manageiq.org/t/can-we-speed-up-small-repos-on-travis/2533
@miq-bot
Copy link
Member

miq-bot commented Jul 17, 2017

Checked commit Fryguy@d64fe19 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@bdunne bdunne merged commit 295c3fd into ManageIQ:master Jul 17, 2017
@bdunne bdunne added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 17, 2017
@cben
Copy link
Contributor

cben commented Jul 18, 2017

Thanks! 🎉
Unfortunately 💥 , I now see provider travises erroring on --path flag (which indeed I dont see in bundle update --help):
https://travis-ci.org/ManageIQ/manageiq-providers-kubernetes/jobs/254369323
https://travis-ci.org/ManageIQ/manageiq-providers-openshift/jobs/254733067
https://travis-ci.org/ManageIQ/manageiq-providers-openstack/jobs/254682962
https://travis-ci.org/ManageIQ/manageiq-providers-ansible_tower/jobs/254638729

Bundler version 1.15.2
...
$ bin/setup
== Cloning manageiq sample app ==
Cloning into 'spec/manageiq'...
Unknown switches '--path=vendor/bundle'
== Command ["bundle update --jobs=3 --retry=3 --path=${BUNDLE_PATH:-vendor/bundle}"] failed in /home/travis/build/ManageIQ/manageiq-providers-kubernetes ==

The command "bin/setup" failed and exited with 1 during .

(manageiq repo still works eg. https://travis-ci.org/ManageIQ/manageiq/jobs/254753082)

@durandom
Copy link
Member

indeed only bundle install has the --path param

@Fryguy
Copy link
Member Author

Fryguy commented Jul 18, 2017

:( This is why we should just stick with what Travis has built in...fix coming soon

@Fryguy Fryguy deleted the fix_bundle_override branch July 18, 2017 14:05
@cben
Copy link
Contributor

cben commented Jul 18, 2017

I think it's important that ManageIQ::Environment PRs get tested on a plugin, in Travis before merging (local can't be good enough, we have ENV['CI'] conditionals, and Travis does its own magic)

  • The simplest idea I can think of is including an "empty plugin" in core repo and testing it.
  • More elegantly, could have a test that runs plugin generator and then tries its bin/setup :) Just need spec/manageiq symlink to use the ManageIQ::Environment code under test instead of checking out master.

Sounds fun, I'll make a PR for something like that.

Another lesson: consider adding some people in non-US timezone with core merge rights — can agree that they'll only use it to fix productivity-breaking things like red master, and/or will only do reverts...

(I also have a bigger plan to make travis read PR description and be able to specify "test this PR with those dependencies" as well as "also test it doesn't break those repos", but that needs more time)

@NickLaMuro
Copy link
Member

Another lesson: consider adding some people in non-US timezone with core merge rights — can agree that they'll only use it to fix productivity-breaking things like red master, and/or will only do reverts...

Since I expect that this would come with some reservations from those dishing out those rights, possible alternative solution is that merging PRs like this should happen in the A.M. instead of near the end of the day. Otherwise you have people shipping and walking away for 12 hours and not being able to fix things and bottleneck others.

Possible a CI-Risk label, or something to that affect, should be added so that Mergers are aware that time should be allotted after a merge to monitor build statuses before stepping away.

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