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

Fixes #37471 - Support Zeitwerk loader #10997

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

ofedoren
Copy link
Contributor

@ofedoren ofedoren commented May 15, 2024

WIP

What are the changes introduced in this pull request?

This PR should make Katello plugin compatible with Zeitwerk autoloader, which will be enabled in Foreman instead of classic Rails' loader.

Considerations taken when implementing this change?

I was trying to change rather less than more. Although, some stuff (like CV acronym) can be introduced, but will require more changes and I'm not sure it'll be quite safe. It's up to maintainers.

What are the testing steps for this pull request?

Test with Foreman main PR (included in CI workflow) and hope for the best. Note: at first it'll be red, since some plugins, which are dependencies, should be updated as well. Additionally, after the tests are green I strongly recommend to test out basic/advanced content related workflows live.

@jeremylenz
Copy link
Member

jeremylenz commented May 23, 2024

I had to check out:

I tested

  • refreshing my manifest
  • deleting a subscription from my manifest
  • enabling a RH repository
  • disabling a RH repository
  • publish and promote a CV
  • remove a package from a custom repository
  • create new blank docker repo
  • syncing all repos

Seems all good so far! I grepped development.log for zeitwerk and "uninitialized constant" and didn't see anything concerning.

@jeremylenz
Copy link
Member

also notable: With the foreman-tasks PR checked out, I did not see this error

Zeitwerk::NameError: expected file /home/runner/work/katello/katello/vendor/bundle/ruby/2.7.0/gems/foreman-tasks-9.1.1/app/lib/actions/middleware/keep_current_request_id.rb to define constant Actions::Middleware::KeepCurrentRequestId, but didn't

which is currently failing in CI.

@ofedoren ofedoren force-pushed the feat-37471-support-zeitwerk branch from 8859c89 to 0c72da8 Compare May 30, 2024 14:56
@ofedoren ofedoren force-pushed the feat-37471-support-zeitwerk branch 2 times, most recently from 2b22e7c to 711dc2b Compare June 24, 2024 15:36
@ofedoren ofedoren marked this pull request as ready for review June 24, 2024 15:36
@ofedoren ofedoren force-pushed the feat-37471-support-zeitwerk branch 4 times, most recently from 66d1f0d to c3daa15 Compare July 8, 2024 15:08
@ofedoren ofedoren force-pushed the feat-37471-support-zeitwerk branch 2 times, most recently from 0f4329a to f30c895 Compare July 30, 2024 10:10
@ofedoren ofedoren force-pushed the feat-37471-support-zeitwerk branch 5 times, most recently from 48f3162 to cb2a1ae Compare September 3, 2024 14:19
@ofedoren
Copy link
Contributor Author

ofedoren commented Sep 3, 2024

Ready to merge after theforeman/foreman#10131 is merged, dropped the commit for testing with Foreman PR, the latest run was green:
Screenshot_20240903_143351

@ekohl
Copy link
Member

ekohl commented Sep 11, 2024

@ofedoren any idea why the tests are failing?

@jeremylenz
Copy link
Member

The failures in test_show and test_show_uuid are unrelated to Zeitwerk; we can merge without fixing those.

@ekohl
Copy link
Member

ekohl commented Sep 11, 2024

@jeremylenz can you merge it then? :)

@jeremylenz jeremylenz merged commit c90a1dc into Katello:master Sep 11, 2024
22 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants