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

Move gem name loader to proper namespaced location. #208

Merged
merged 1 commit into from
May 2, 2023

Conversation

jrafanie
Copy link
Member

Move gem name loader to proper namespaced location.

@kbrock
Copy link
Member

kbrock commented Apr 22, 2023

I think the proper solution is to create lib/manageiq-appliance_console.rb that requires manageiq/appliance_console

This is the pattern I tend to see in gems.

@jrafanie
Copy link
Member Author

@kbrock yeah, I understand that is/was the standard for non-rails stuff but zeitwerk is adopting the fairly new (maybe last several years) standard of lib/manageiq/appliance_console.rb, see: https://guides.rubygems.org/name-your-gem/

Checkout https://github.com/fxn/zeitwerk/issues 138 for more background.

Basically, zeitwerk is expecting files to define the constants based on their name and these "gem name.rb" loader files in lib fail this test. I'd like to find a way to get zeitwerk to work with them without needing to continually tell zeitwerk to ignore them but for now, this is the easiest way.

Basically, unless we expect requires to use the manageiq-appliance_console format, we can safely let bundler do the require as it will try the hyphen form, and if that doesn't exsit, it will use the manageiq/appliance_console form of require.

@kbrock
Copy link
Member

kbrock commented Apr 24, 2023

I kicked but I think https://github.com/ManageIQ/manageiq-appliance_console/blob/master/Gemfile having 2 source lines is an issue.

@kbrock
Copy link
Member

kbrock commented Apr 26, 2023

searched all the code. Looks like the only references to require "manageiq-appliance_console" are from within this gem.

@kbrock
Copy link
Member

kbrock commented Apr 26, 2023

kicking to start the build again.

@kbrock kbrock closed this Apr 26, 2023
@kbrock kbrock reopened this Apr 26, 2023
@jrafanie jrafanie closed this Apr 26, 2023
@jrafanie jrafanie reopened this Apr 26, 2023
@kbrock
Copy link
Member

kbrock commented Apr 28, 2023

@jrafanie I added #209 to fix the build.
After we merge that, lets see if this turns green.

@jrafanie jrafanie closed this Apr 28, 2023
@jrafanie jrafanie reopened this Apr 28, 2023
@miq-bot
Copy link
Member

miq-bot commented Apr 28, 2023

Checked commit jrafanie@e35bfbb with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. 👍

@kbrock kbrock merged commit befefc2 into ManageIQ:master May 2, 2023
@jrafanie jrafanie deleted the zeitwerk_gem_name_loader branch May 2, 2023 15:15
jrafanie pushed a commit that referenced this pull request Jun 30, 2023
Move gem name loader to proper namespaced location.

(cherry picked from commit befefc2)
Fryguy added a commit that referenced this pull request Feb 7, 2024
Fixed
- Fix sporadic test failure [#204]
- Remove MIQ specific gem source [#209]
- Double escape @ in realm to avoid shell interpretation [#211]
- Move gem name loader to proper namespaced location [#208]
- Separate kerberos from service principal name and use correctly [#215]
- Add manageiq user to allowed_uids for sssd [#220]
- Remove warning about using pg_dump [#221]
- Fix specs where AwesomeSpawn private interface changed [#224]
- Change the Name of the CA from something to ApplianceCA [#228]
- Fix YAML.load_file failing on aliases [#234]

Added
- Make backward compatible changes to work with repmgr13 - version 5.2.1 [#192]
- Support Ruby 3.0 [#206]
- Support Ruby 3.1 [#227]
- Allow rails 7 gems in gemspec [#226]

Changed
- Update to Highline 2.1.0 [#201]
- Clean up test output (highline and stdout messages) [#210]

Removed
- Drop Ruby 2.7 [#223]
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.

4 participants