theforeman / forklift

Helpful deployment scripts for Foreman and Katello
https://theforeman.github.io/forklift
GNU General Public License v3.0
183 stars 199 forks source link

Clone test gems to katello.local.rb to allow local katello testing #1835

Closed qcjames53 closed 1 month ago

qcjames53 commented 1 month ago

This PR addresses SAT-26017, adding katello test gems to foreman's katello.local.rb on vagrant box provision if certain conditions are met.

Considerations: As of now, customize_home is only called for katello-related boxes. I added checks for both the source dir and destination file existing just in case. They shouldn't kick in unless we decide to customize_home for non-katello boxes as well, but I figured it's better to be safe.

Testing Steps:

  1. In the forklift dir, create a new katello-devel box and provision it. On completion, check the contents of ~/foreman/bundler.d/katello.local.rb. It should a 'test' group similar to the following:
    group :test do
    gem "vcr", "~> 6.1"
    gem 'theforeman-rubocop', '~> 0.0.6', require: false
    end
  2. Re-run the provisioning on this box without destroying. ~/foreman/bundler.d/katello.local.rb should contain only one 'test' group.
  3. Create a katello-devel-stable box. ~/foreman/bundler.d/katello.local.rb should still contain a 'test' group.
qcjames53 commented 1 month ago

Hi William, thanks for the review! I rewrote the tasks following your suggestions; it's certainly a lot cleaner now. I didn't end up testing the stable box for this specific push but I can confirm it functions ok for creating regular katello-devel boxes.

wbclark commented 1 month ago

Hey @qcjames53 , your new implementation is indeed what I had in mind, but I think @ekohl has a good point -- it would be much better still to have ruby read the files on the fly instead of doing it once during provisioning, so even if there are changes we can just run a bundle install without having to re-run the whole provisioning.

It's a good reminder to both of us to think carefully about the problem before accepting the proposed solution in a ticket.

wbclark commented 1 month ago

Another thought is that the customize_home role exists to customize the user's home directory and is probably not the best fitting place for this functionality to live. What to do about that?

One idea is that while the katello_devel role is currently a meta role that functions only through its dependencies:

$ tree roles/katello_devel/
roles/katello_devel/
└── meta
    └── main.yml

2 directories, 1 file

It doesn't have to be, and we could give it this structure:

$ tree roles/katello_devel/
roles/katello_devel/
├── files
│   └── katello-test.local.rb
├── meta
│   └── main.yml
└── tasks
    └── main.yml

4 directories, 3 files

But, I think doing it that way is still not quite right, because it would require doing a second bundle install when we already do that at an earlier stage here: https://github.com/theforeman/puppet-katello_devel/blob/master/manifests/setup.pp#L27-L29 (which executes while the installer is running)

So probably the very best approach would be the same general idea as above, but doing it within the puppet module directly instead of in forklift, for example by creating a new file resource immediately after we create the katello repository here: https://github.com/theforeman/puppet-katello_devel/blob/master/manifests/config.pp#L47-L51

Before I send you on any more wild goose chases, let's ask if this plan sounds good to @ekohl

ekohl commented 1 month ago

We could enhance the generated bundler.d file to always include the test dependencies. That mirrors our CI so is probably the right thing anyway. And we should then do it for every plugin

ekohl commented 1 month ago

To elaborate on that: https://github.com/theforeman/puppet-katello_devel/blob/master/templates/plugin.local.rb.erb can be extended to also look for gemfile.d in the same path as the plugin itself.

wbclark commented 1 month ago

@qcjames53 is planning to open a new PR in puppet-katello_devel adding support for test gems in the plugin defined type. I'll close this one in the meantime. Thanks to you both!