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

support aliases in hiera yaml #144

Merged
merged 3 commits into from
Jul 7, 2023
Merged

Conversation

akerl
Copy link
Contributor

@akerl akerl commented Jun 19, 2023

This enables support for Hiera YAML files that contain aliases. As an example, with the current version I see this error:

❯ bundle exec rake syntax
---> syntax:manifests
---> syntax:templates
---> syntax:hiera:yaml
ERROR: Failed to parse data/mac/2c:4d:54:4f:be:c3.yaml: Alias parsing was not enabled. To enable it, pass `aliases: true` to `Psych::load` or `Psych::safe_load`.

This is the file it's referring to: https://github.com/halyard/halyard/blob/main/data/mac/2c%3A4d%3A54%3A4f%3Abe%3Ac3.yaml#L43

This PR adds the aliases: true flag, which seems right to me given that Puppet/Hiera allow aliases when parsing YAML.

@@ -74,7 +74,7 @@ def check(filelist)

filelist.each do |hiera_file|
begin
yamldata = YAML.load_file(hiera_file)
yamldata = YAML.load_file(hiera_file, aliases: true)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you need to do some version detection if the aliases keyword is supported.

@akerl
Copy link
Contributor Author

akerl commented Jun 19, 2023

Good catch. I think I've fixed it. 2 remaining failures seem to be expected, since the latest Puppet doesn't work w/ older Ruby versions.

@bastelfreak
Copy link
Member

Hi, please rebase after #145 got merged

@akerl
Copy link
Contributor Author

akerl commented Jul 7, 2023

Updated!

@@ -74,7 +74,8 @@ def check(filelist)

filelist.each do |hiera_file|
begin
yamldata = YAML.load_file(hiera_file)
yamlargs = RUBY_VERSION >= '3.1' ? { aliases: true } : {}
Copy link
Member

Choose a reason for hiding this comment

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

Should this look at Psych::VERSION instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It felt more hacky to pin to an internal library version vs the more top-level Ruby version

Copy link
Member

Choose a reason for hiding this comment

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

Can't you update Psych via dependencies? Either way this is still pretty hacky and it's not going to get cleaner so I'm fine either way. I guess it's fine either way.

Copy link
Member

Choose a reason for hiding this comment

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

And as I press submit, perhaps the yamlargs should be defined outside of the filelist.each loop. I really hope it's not going to change in between runs.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @ekohl here. Ruby bundles a Psych version, but it can be updated. So we should check for the Psych version, not the Ruby version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to be outside the loop and use Psych::VERSION

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Thanks!

@ekohl ekohl merged commit 0927f04 into voxpupuli:master Jul 7, 2023
10 checks passed
@bastelfreak bastelfreak added the enhancement New feature or request label Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants