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

Seed Ansible Roles for Vmdb Plugins #17777

Merged
merged 6 commits into from
Aug 2, 2018

Conversation

agrare
Copy link
Member

@agrare agrare commented Jul 30, 2018

Allow plugins to add a requirements.yml file to specify roles from
galaxy/github/etc... and seed them locally.

If I have an example requirements.yml:

cat content_tmp/ansible/requirements.yml 
- src: yatesr.timezone

And I seed the roles

bundle exec rake evm:ansible_runner:seed
** override_gem: manageiq-providers-vmware, [{:path=>"/home/agrare/src/manageiq/manageiq-providers-vmware"}], caller: /home/agrare/src/manageiq/manageiq/bundler.d/plugins.rb
Seeding roles for ManageIQ::Providers::Vmware::Engine...
Seeding roles for ManageIQ::Providers::Vmware::Engine...Complete

ls -l ../manageiq-providers-vmware/content_tmp/ansible/roles/
total 0
drwxr-xr-x 1 agrare agrare  78 Jul 30 11:08 yatesr.timezone

And an example failure:

>../manageiq-providers-vmware/content_tmp/ansible/requirements.yml

bundle exec rake evm:ansible_runner:seed
Seeding roles for ManageIQ::Providers::Vmware::Engine...
Seeding roles for ManageIQ::Providers::Vmware::Engine...Failed - ERROR! No roles found in file: /home/agrare/src/manageiq/manageiq-providers-vmware/content_tmp/ansible/requirements.yml
rake aborted!
AwesomeSpawn::CommandResultError: ansible-galaxy exit code: 1

@agrare
Copy link
Member Author

agrare commented Jul 30, 2018

cc @gtanzillo @Ladas

Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

Should be good 👍

We should probably extract the path to ansible to core, if this script expects it in some location, right?

And rename the content_tmp :-)

@agrare
Copy link
Member Author

agrare commented Jul 30, 2018

We should probably extract the path to ansible to core, if this script expects it in some location, right?

And rename the content_tmp :-)

Yeah I was trying to think of a good place to put this constant since eventually it'll be shared by both Ansible::Runner and EmbeddedAnsible but figured I'd put the PR in and we could discuss here :)

@agrare agrare force-pushed the seed_ansible_roles_for_providers branch from 3342869 to 7f00d5a Compare July 31, 2018 18:36
Allow plugins to add a requirements.yml file to specify roles from
galaxy/github/etc... and seed them locally.
@agrare agrare force-pushed the seed_ansible_roles_for_providers branch 2 times, most recently from 91abf5c to 8536545 Compare August 2, 2018 14:06
@agrare agrare force-pushed the seed_ansible_roles_for_providers branch from 8536545 to 636e0a6 Compare August 2, 2018 14:12
@agrare
Copy link
Member Author

agrare commented Aug 2, 2018

Okay @gtanzillo @Fryguy this is ready to review
We still need to figure out what we're doing with the content/ansible collision with playbook seeding. Don't need to do that in this PR just pointing it out 😄

desc "Seed galaxy roles for provider playbooks"
task :seed do
plugins_with_req_yml = Vmdb::Plugins.select do |plugin|
req_yml_path = plugin.root.join('content_tmp', 'ansible', 'requirements.yml')
Copy link
Member

Choose a reason for hiding this comment

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

Let's encode this into VMDB::Plugins a la VMDB::Plugins::AnsibleContent

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 done

puts "Seeding roles for #{plugin.name}..."

roles_path = plugin.root.join('content_tmp', 'ansible', 'roles')
role_file = plugin.root.join('content_tmp', 'ansible', 'requirements.yml')
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer 'content', 'ansible_tmp', .... content won't change, but the inner dir might.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

nil => "install",
:roles_path= => roles_path,
:role_file= => role_file
}
Copy link
Member

Choose a reason for hiding this comment

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

This can also be

params = ["install", :roles_path= => roles_path, :role_file= => role_file]

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@agrare agrare force-pushed the seed_ansible_roles_for_providers branch from 5d79d37 to 71e06cb Compare August 2, 2018 17:17
@agrare agrare force-pushed the seed_ansible_roles_for_providers branch from 71e06cb to 383a039 Compare August 2, 2018 17:27
@agrare agrare force-pushed the seed_ansible_roles_for_providers branch from 9668a36 to 1e76650 Compare August 2, 2018 17:54
@miq-bot
Copy link
Member

miq-bot commented Aug 2, 2018

Some comments on commits agrare/manageiq@d2b0a70~...1e76650

lib/tasks/evm_ansible_runner.rake

  • ⚠️ - 18 - Detected puts. Remove all debugging statements.
  • ⚠️ - 20 - Detected puts. Remove all debugging statements.
  • ⚠️ - 9 - Detected puts. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented Aug 2, 2018

Checked commits agrare/manageiq@d2b0a70~...1e76650 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@gtanzillo gtanzillo added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 2, 2018
@gtanzillo gtanzillo merged commit 4eb1471 into ManageIQ:master Aug 2, 2018
@agrare agrare deleted the seed_ansible_roles_for_providers branch August 2, 2018 18:22
@ansible_runner_content ||= begin
map do |engine|
content_dir = engine.root.join("content", "ansible_tmp")
next unless File.exist?(content_dir.join("requirements.yml"))
Copy link
Member

Choose a reason for hiding this comment

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

My only concern here is that this enforces a requirements.yml. Is that ok? If it's only to be used for the purpose of galaxy stuff, then maybe we just need to rename the method, or return both things that have non-galaxy and galaxy content, if that makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

requirements.yml came from the galaxy docs https://docs.ansible.com/ansible/latest/reference_appendices/galaxy.html#installing-multiple-roles-from-a-file but you can use that file to seed roles from more than just galaxy. You can also include other files from within this file. I don't think you can specify multiple role files, we would have to run ansible-galaxy for each *.yml file if we wanted to do that.

content_dir = engine.root.join("content", "ansible_tmp")
next unless File.exist?(content_dir.join("requirements.yml"))

[engine, content_dir]
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer if this returned a Hash like some of the other ones here. Either that or create the wrapper struct like AutomateDomain or AnsibleContent.

@ansible_runner_content ||= begin
map do |engine|
content_dir = engine.root.join("content", "ansible_tmp")
next unless File.exist?(content_dir.join("requirements.yml"))
Copy link
Member

@Fryguy Fryguy Aug 2, 2018

Choose a reason for hiding this comment

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

Can we reuse the content_directories method? EDIT: nvm...content_directories assumes there are multiple things inside, which in this case isn't true.

Copy link
Member Author

@agrare agrare Aug 2, 2018

Choose a reason for hiding this comment

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

content_directories would do content/ansible_tmp/* which would return every file and dir in that directory. For the purpose of seeding we only care about the role_file. We could change content_directories to take the last component as an arg and default to * but we could override with .e.g. requirements.yml

Ah didn't see your edit

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.

5 participants