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

change ManageIQ::Environment to run bundle install on plugin_setup #15589

Merged
merged 2 commits into from
Jul 18, 2017

Conversation

durandom
Copy link
Member

  • only pass bundle_params for bundle install
  • change plugin_setup task to run bundle install
  • add plugin_update task to run bundle update

continued from #15508 I think its worth to add an manageiq_plugin_update method, as it seems to be different to run bundle install vs bundle update.

reverts parts of #15585
similar to the revert in #15588

@Fryguy @bdunne @cben

* change plugin_setup task to run bundle install
* add plugin_update task to run bundle update
@@ -19,6 +19,15 @@ def self.manageiq_plugin_setup
setup_test_environment(:task_prefix => 'app:', :root => plugin_root)
end

def self.manageiq_plugin_update
# determine plugin root dir. Assume we are called from a 'bin/setup' script in the plugin root
plugin_root = Pathname.new(caller_locations.last.absolute_path).dirname.parent
Copy link
Contributor

Choose a reason for hiding this comment

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

way too magic IMHO., please pass a dir argument. bin/setup etc should be debuggable by everyone, focusing on commands that it actually runs, not ruby magic...

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed, I did it that way so that we dont have to change bin/setup in the provider repos.

I'll add a default arg though

@miq-bot
Copy link
Member

miq-bot commented Jul 18, 2017

Checked commits durandom/manageiq@81635a3~...4c87a46 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

@cben
Copy link
Contributor

cben commented Jul 18, 2017

Overall makes sense, though I think we'd still want at least --jobs=3 for bin/update?
Some alternatives I tried that don't work well:

  • bundle update without --path: works but seems to break caching(?), fetches all gems
    https://travis-ci.org/cben/manageiq-providers-kubernetes/jobs/254778155
    added a bundle show => gems got installed in e.g. /home/travis/.rvm/gems/ruby-2.3.3/bundler/gems/manageiq-providers-openshift-f2105258d580

  • setting BUNDLE_PATH env var instead of --path => other commands like bin/rails app:test:vmdb:setup don't find the gems :-( Perhaps if we set BUNDLE_PATH in .travis.yml ?

https://docs.travis-ci.com/user/caching/#Determining-the-bundle-path has some (not very clear) explanations on BUNDLE_PATH - caching interaction.

@durandom
Copy link
Member Author

durandom commented Jul 18, 2017 via email

@cben
Copy link
Contributor

cben commented Jul 18, 2017

I know, I was thinking of also enabling --jobs=3 locally.
But on my computer I'm seeing bundle update takes 30+sec, bundle update --jobs=3 takes 50+ sec so better not :-)

@@ -11,4 +11,4 @@ else
end

require gem_root.join("spec/manageiq/lib/manageiq/environment").to_s
ManageIQ::Environment.manageiq_plugin_setup
ManageIQ::Environment.manageiq_plugin_setup(gem_root)
Copy link
Member

Choose a reason for hiding this comment

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

I think you meant for this to call manageiq_plugin_update?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, indeed :(

@Fryguy
Copy link
Member

Fryguy commented Jul 18, 2017

Merging on red since the errors are unrelated.

@durandom Since we are not using the bin/update script in Travis, I'm merging, but you may need to change for #15589 (comment)

@Fryguy Fryguy merged commit 703b6d6 into ManageIQ:master Jul 18, 2017
@Fryguy Fryguy added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 18, 2017
@cben
Copy link
Contributor

cben commented Jul 18, 2017

Confirmed, https://travis-ci.org/ManageIQ/manageiq-providers-openshift/jobs/254862190 passed bin/setup 🎉 (that PR is gonna fail tests, that's expected)

@durandom durandom deleted the bundle_env branch July 18, 2017 18:45
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