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

manageiq env plugins should update everything on bin/update #15508

Merged
merged 2 commits into from
Jul 6, 2017

Conversation

durandom
Copy link
Member

@durandom durandom commented Jul 5, 2017

provider devs need to go through some manual commands to update their env to the latest miq branch. I.e. they would need to run bin/update; bundle update; rake spec:setup

this PR

  • changes manageiq_plugin_setup to run bundle update --> bin/update will actually update your dependencies
  • make the rake task test:vmdb:setup work when run from app:test:vmdb:setup
  • thus removes the need to have a Gemfile.lock inside spec/manageiq because no rake task is run from within this directory

see ManageIQ/manageiq-providers-ansible_tower#11 how this would work in a provider

@@ -9,6 +9,7 @@ env:
- RUBY_GC_HEAP_GROWTH_MAX_SLOTS=300000
- RUBY_GC_HEAP_INIT_SLOTS=600000
- RUBY_GC_HEAP_GROWTH_FACTOR=1.25
bundler_args: --no-deployment
Copy link
Member Author

Choose a reason for hiding this comment

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

@Fryguy @NickLaMuro is this needed?
I dont really follow ManageIQ/manageiq-content#136

Copy link
Member

Choose a reason for hiding this comment

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

Short answer: yes, because we are now using a path gem with mime-types-redirector, and bundling twice and using --deployment (which is a Travis default) is causing issues with that gem.

Copy link
Member

Choose a reason for hiding this comment

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

👎 I want this to not exist, and would prefer we fix the double-bundle-update instead. See also #15503

Copy link
Member Author

Choose a reason for hiding this comment

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

@NickLaMuro

bundling twice

can you point me to where this happens?
Maybe this does not happen anymore with the changes in this PR?

See the travis log for ManageIQ/manageiq-providers-ansible_tower#11
I dont see running bundle install twice there

Copy link
Member

Choose a reason for hiding this comment

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

I want this to not exist, and would prefer we fix the double-bundle-update instead. See also #15503

That is fair, that is the other option, and that works as well.

bundling twice
can you point me to where this happens?

@durandom If you expand the output of the bin/setup, you will see the bundle install output there. That happens prior to the one that Travis gives by default.

Copy link
Member

@Fryguy Fryguy Jul 5, 2017

Choose a reason for hiding this comment

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

https://github.com/ManageIQ/manageiq/pull/15508/files#diff-7dbc55ccfbebe14a0aa870a17989c558R29 is the first bundle that would occur in bin/setup. Then, you get a second bundle update as the default "install" step on travis. Specifically, under the covers it does bundle install --jobs=3 --retry=3 --deployment, and the reason you get the --deployment bit is because the Gemfile.lock exists after the first bundle install.

See https://docs.travis-ci.com/user/languages/ruby/#Travis-CI-uses-Bundler
and https://docs.travis-ci.com/user/customizing-the-build/#Skipping-the-Installation-Step

Copy link
Member Author

@durandom durandom Jul 5, 2017

Choose a reason for hiding this comment

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

@NickLaMuro @Fryguy I still dont see the second bundle install. The first one is initiated by bin/setup, ok. But where is the "default" one? It's not in the output.
Also, reading the docs, that @Fryguy linked

If you want to use a different means of handling your Ruby project’s dependencies, you can override the install command.

Thats what we do. here

EDIT:
and I'm looking at the ansible tower PR log: https://travis-ci.org/ManageIQ/manageiq-providers-ansible_tower/jobs/250387157

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevertheless, I'll remove that commit here, as it's not wanted.

Copy link
Member Author

Choose a reason for hiding this comment

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

reading back to ManageIQ/manageiq-ui-classic#1606 (comment) it seems, this only affects the ui and miq-content repo.
As the provider repos use install: bin/setup in .travis.yml and the others dont.

I think we should streamline this and all plugins should use bin/setup as the install

@durandom
Copy link
Member Author

durandom commented Jul 5, 2017

@bdunne please have a look

setup_test_environment(:task_prefix => 'app:', :root => plugin_root)
end

def self.manageiq_plugin_update
Copy link
Member

Choose a reason for hiding this comment

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

What does this buy us? Just the bundle update? If that's the case, I'd rather see manageiq_plugin_setup always bundle update so that we don't have duplicate logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was following along bin/setup and bin/update in the core, which do bundle install and bundle update. But 👍 for simplicity. Will drop the update

durandom added 2 commits July 6, 2017 11:59
if so, the dependencies are also run namespaced
this is used from a plugins 'spec:setup' task and thus moves the
dependencies properly to the core
@durandom durandom force-pushed the generator_no_deployment branch from 9a309f5 to cf2bab3 Compare July 6, 2017 10:07
@durandom
Copy link
Member Author

durandom commented Jul 6, 2017

@bdunne have a look again. I also updated the PR description

@miq-bot
Copy link
Member

miq-bot commented Jul 6, 2017

Checked commits durandom/manageiq@93bcfaa~...cf2bab3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@bdunne bdunne merged commit 1960851 into ManageIQ:master Jul 6, 2017
@bdunne bdunne added this to the Sprint 64 Ending Jul 10, 2017 milestone Jul 6, 2017
@bdunne bdunne added the fine/no label Jul 6, 2017
@durandom durandom deleted the generator_no_deployment branch July 6, 2017 13:38
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.

6 participants