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

(PUP-10861) Don't JSON.dump single fact values #8500

Merged
merged 2 commits into from Feb 2, 2021

Conversation

joshcooper
Copy link
Contributor

The puppet facts show --value-only option prints just the value for a single
fact. Previously, the value was pretty printed using JSON.dump, which emitted a
quoted string. Now just print the string, so that the value can be parsed via
automation, like export OSFAMILY=puppet facts show os.family --value-only

Also add a test for multiple fact names as the --value-only option.

Use squiggly heredoc so it's clearer what output we're expecting.
@joshcooper joshcooper requested review from a team January 28, 2021 23:30
result
else
Puppet::Util::Json.dump(result, :pretty => true)
end
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 looks like facter can emit [Integer, Float, TrueClass, FalseClass, NilClass, Symbol, String, Array, Hash], so I'm thinking we only want to call Json.dump for Array and Hash, and rely on to_s for the other types?

Copy link
Contributor

Choose a reason for hiding this comment

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

@joshcooper I think for NilClass JSON.dump and to_s behave differently

JSON.dump(nil) #=> "null"
nil.to_s   #=> ""

But i'm not sure if Facter will return any nil value, or it will convert it before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Facter can return nil, so I updated this (including other types in https://github.com/puppetlabs/facter/blob/0654f8f6858ca0163680b3d8dfc53d90345f0f35/lib/facter/custom_facts/util/normalization.rb#L8)

I filed https://tickets.puppetlabs.com/browse/FACT-2930 as it seems facter can emit a Date (which seems like a mistake), but the error message about VALID_TYPES is inconsistent.

@joshcooper joshcooper force-pushed the dont_quote_values_10861 branch 2 times, most recently from 6c53d26 to 60d0d22 Compare January 29, 2021 20:09
@joshcooper
Copy link
Contributor Author

grr... JRuby output is different, I'll update that.

The `puppet facts show --value-only` option prints just the value for a single
fact. Previously, the value was pretty printed using JSON.dump, which emitted a
quoted string. Now just print the string, so that the value can be parsed via
automation, like export OSFAMILY=`puppet facts show os.family --value-only`

Also add a test for multiple fact names as the `--value-only` option.
@headius
Copy link

headius commented Feb 1, 2021

@joshcooper Differing output may be grounds for a JRuby bug. If whatever it was seems like it should match CRuby, could you please open a JRuby issue? https://github.com/jruby/jruby/issues

@joshcooper joshcooper merged commit c11abe4 into puppetlabs:main Feb 2, 2021
@joshcooper
Copy link
Contributor Author

Thanks @headius! FWIW, JRuby output has an extra empty line when pretty printing, but the output is still correct, so I think it's ok? If you want it to be exactly the same, I can file a ticket?

$ bundle exec ruby -rjson -e "puts RUBY_DESCRIPTION; puts JSON.pretty_generate({})"
ruby 2.5.8p224 (2020-03-31 revision 67882) [x86_64-darwin18]
{
}
$ bundle exec ruby -rjson -e "puts RUBY_DESCRIPTION; puts JSON.pretty_generate({})"
jruby 9.2.9.0 (2.5.7) 2019-10-30 458ad3e Java HotSpot(TM) 64-Bit Server VM 25.162-b12 on 1.8.0_162-b12 +jit [darwin-x86_64]
{

}

@joshcooper joshcooper deleted the dont_quote_values_10861 branch February 2, 2021 16:36
@headius
Copy link

headius commented Feb 2, 2021

@joshcooper Aha, now that does ring a bell.

So assuming this is the same issue, I think it will be resolved once the updated json gem is being used everywhere.

@headius
Copy link

headius commented Feb 2, 2021

JRuby 9.2.15.0 and 9.3.0.0 will ship the updated json.

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.

None yet

3 participants