-
Notifications
You must be signed in to change notification settings - Fork 28
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
Fix YAML.load_file failing on aliases #234
Fix YAML.load_file failing on aliases #234
Conversation
YAML.load_file(DB_YML) | ||
|
||
if YAML.respond_to?(:safe_load_file) | ||
YAML.safe_load_file(DB_YML, :aliases => true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, safe_load_file was not available until a specific version of psych and it was easier to check for safe_load, which came first and use that along with File.read.
safe_load_file was added in 3.2.1 ruby/psych@0210e31
unsafe_load and unsafe_load_file was added in 3.3.2:
ruby/psych@cb50aa8
safe_load was added with positional arguments in 2.0.0:
ruby/psych@2c644e1
At some point it changed to having kwargs. It's a mess.
I know, too much information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😱 okay I'll match what you have in messaging_configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, I kept digging, kwargs for aliases
was added in 3.1.0: ruby/psych@4d4439d. I made the assumption we'd be at 3.1.0 at least so we'll have safe_load
with kwargs but perhaps not yet have safe_load_file
or unsafe_load
/unsafe_load_file
.
Some gems support super old rubies so it was easier to just follow that pattern. Technically, it wouldn't work with psych 3 or earlier but 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we enforce a minimum of 3.1.0? That way we know we have aliases as kwargs?
Fix configuring an internal database with ruby 3.1 failing due to aliases in the `config/database.pg.yml` file ``` /opt/manageiq/manageiq-gemset/gems/psych-4.0.6/lib/psych/visitors/to_ruby.rb:432:in 'visit_Psych_Nodes_Alias': Unknown alias: base (Psych::BadAlias) ```
fb4eb25
to
485e72c
Compare
Checked commit agrare@485e72c with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, complexity is failing but this method is not even that bad. 🤣
Followup to discussion in ManageIQ/manageiq-appliance_console#234 3.1 safe_load changed positional to kwargs like aliases: true: ruby/psych@4d4439d Note, there are other changes to psych over time that ease the changes to the psych 4 defaults to safe_load but this is the absolute minimum we need. safe_load_file was added in 3.2.1: ruby/psych@0210e31 unsafe_load and unsafe_load_file was added in 3.3.2: ruby/psych@cb50aa8 safe_load was added with positional arguments in 2.0.0: ruby/psych@2c644e1
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]
Fix configuring an internal database with ruby 3.1 failing due to aliases in the
config/database.pg.yml
fileFixes #232