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

Avoid seeding ansible content in production #21089

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Mar 2, 2021

Ansible content is already seeded at RPM build time, so to avoid the
need to hit the filesystem again, we should skip this in production
mode.

Additionally, this adds the consolidation of ansible content to seed
time, in addition to the fetching of galaxy roles.

@kbrock @agrare Please review.

Note that the seeding happens at RPM build time here: https://github.com/ManageIQ/manageiq-rpm_build/blob/3173133b27375a26cc205c09d3d3c627e907c635/lib/manageiq/rpm_build/generate_core.rb#L63-L67

Ansible content is already seeded at RPM build time, so to avoid the
need to hit the filesystem again, we should skip this in production
mode.

Additionally, this adds the consolidation of ansible content to seed
time, in addition to the fetching of galaxy roles.
@miq-bot
Copy link
Member

miq-bot commented Mar 2, 2021

Some comments on commit Fryguy@6f40eb1

lib/ansible/content.rb

  • ⚠️ - 22 - Detected puts. Remove all debugging statements.
  • ⚠️ - 25 - Detected puts. Remove all debugging statements.
  • ⚠️ - 27 - Detected puts. Remove all debugging statements.

lib/tasks/evm_ansible_runner.rake

  • ⚠️ - 11 - Detected puts. Remove all debugging statements.
  • ⚠️ - 13 - Detected puts. Remove all debugging statements.
  • ⚠️ - 7 - Detected puts. Remove all debugging statements.
  • ⚠️ - 9 - Detected puts. Remove all debugging statements.

@Fryguy
Copy link
Member Author

Fryguy commented Mar 2, 2021

Moving forward we may want to split the seeding task into 2 separate tasks and run them separately at build time (consolidate for core and fetch_galaxy for gemset), but this should be fine for now.

@miq-bot
Copy link
Member

miq-bot commented Mar 2, 2021

Checked commit Fryguy@6f40eb1 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
4 files checked, 3 offenses detected

lib/ansible/content.rb

  • ❗ - Line 22, Col 9 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.
  • ❗ - Line 25, Col 11 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.
  • ❗ - Line 27, Col 11 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.

end
end

def self.consolidate_plugin_content(dir = PLUGIN_CONTENT_DIR)
Copy link
Member Author

Choose a reason for hiding this comment

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

Note the method rename here. This was intentional, as we don't just consolidate playbooks, so the name was inaccurate and confusing.

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

this is nice jason

# In development mode, changes to content will be reconsolidated on every
# seed for convenience.
unless Rails.env.production?
Ansible::Content.consolidate_plugin_content
Copy link
Member

Choose a reason for hiding this comment

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

still feel this would be better suited for /bin/update or /bin/setup

but I'm ok with this

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I didn't want to rock this boat too much, so I left it...we can definitely debate dropping it from here, but I'd rather do that in a follow up.

I really don't understand the use case of why it's here in seed in the first place. From what I can tell it's for developers modifying things, but then, I don't see why they can't just call evm:ansible_runner:seed themselves.

@kbrock
Copy link
Member

kbrock commented Mar 4, 2021

Moving forward we may want to split the seeding task into 2 separate tasks and run them separately at build time (consolidate for core and fetch_galaxy for gemset), but this should be fine for now.

I was hoping we wouldn't need anything in core runtime - only core setup/install

@kbrock kbrock merged commit 784d4f2 into ManageIQ:master Mar 4, 2021
@Fryguy Fryguy deleted the ansible_seeding branch March 4, 2021 19:24
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.

3 participants