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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a8626f
(PUP-10861) Use heredoc for readability
joshcooper Jan 28, 2021
0de8cfc
(PUP-10861) Don't JSON.dump single fact values
joshcooper Jan 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions lib/puppet/face/facts.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,17 +151,23 @@
options[:resolve_options] = true
result = Puppet::Node::Facts.indirection.find(Puppet.settings[:certname], options)

facts = result.values

if options[:value_only]
facts.values.first
result.values.values.first
else
facts
result.values
end
end

when_rendering :console do |result|
Puppet::Util::Json.dump(result, :pretty => true)
# VALID_TYPES = [Integer, Float, TrueClass, FalseClass, NilClass, Symbol, String, Array, Hash].freeze
# from https://github.com/puppetlabs/facter/blob/4.0.49/lib/facter/custom_facts/util/normalization.rb#L8

case result
when Array, Hash
Puppet::Util::Json.dump(result, :pretty => true)
else # one of VALID_TYPES above
result
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.

end
end
end
Expand Down
59 changes: 55 additions & 4 deletions spec/unit/application/facts_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,12 @@
end

context 'when show action is called' do
let(:expected) { "{\n \"filesystems\": \"apfs,autofs,devfs\",\n \"macaddress\": \"64:52:11:22:03:25\"\n}\n" }
let(:expected) { <<~END }
{
"filesystems": "apfs,autofs,devfs",
"macaddress": "64:52:11:22:03:25"
}
END

before :each do
Puppet::Node::Facts.indirection.terminus_class = :facter
Expand All @@ -64,12 +69,58 @@
expect {
app.run
}.to exit_with(0)
.and output(expected).to_stdout
.and output(expected).to_stdout
end

it 'displays a single fact value' do
app.command_line.args << 'filesystems' << '--value-only'
expect {
app.run
}.to exit_with(0)
.and output("apfs,autofs,devfs\n").to_stdout
end

it "warns and ignores value-only when multiple fact names are specified" do
app.command_line.args << 'filesystems' << 'macaddress' << '--value-only'
expect {
app.run
}.to exit_with(0)
.and output(expected).to_stdout
.and output(/it can only be used when querying for a single fact/).to_stderr
end

{
"type_hash" => [{'a' => 2}, "{\n \"a\": 2\n}"],
"type_array" => [[], "[\n\n]"],
"type_string" => ["str", "str"],
"type_int" => [1, "1"],
"type_float" => [1.0, "1.0"],
"type_true" => [true, "true"],
"type_false" => [false, "false"],
"type_nil" => [nil, ""],
"type_sym" => [:sym, "sym"]
}.each_pair do |name, values|
it "renders '#{name}' as '#{values.last}'" do
fact_value = values.first
fact_output = values.last

allow(Facter).to receive(:resolve).and_return({name => fact_value})

app.command_line.args << name << '--value-only'
expect {
app.run
}.to exit_with(0)
.and output("#{fact_output}\n").to_stdout
end
end
end

context 'when default action is called' do
let(:expected) { "---\nfilesystems: apfs,autofs,devfs\nmacaddress: 64:52:11:22:03:25\n" }
let(:expected) { <<~END }
---
filesystems: apfs,autofs,devfs
macaddress: 64:52:11:22:03:25
END

before :each do
Puppet::Node::Facts.indirection.terminus_class = :facter
Expand All @@ -81,7 +132,7 @@
expect {
app.run
}.to exit_with(0)
.and output(expected).to_stdout
.and output(expected).to_stdout
expect(app.action.name).to eq(:show)
end
end
Expand Down