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

(CONT-675) Fix fact detection #96

Merged
merged 1 commit into from
Feb 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 12 additions & 6 deletions lib/puppet-lint/plugins/legacy_facts/legacy_facts.rb
Original file line number Diff line number Diff line change
Expand Up @@ -115,19 +115,25 @@ def check
tokens.select { |x| LEGACY_FACTS_VAR_TYPES.include?(x.type) }.each do |token|
fact_name = ''

# Get rid of the top scope before we do our work. We don't need to
# preserve it because it won't work with the new structured facts.
if token.value.start_with?('::')
fact_name = token.value.sub(%r{^::}, '')
# This matches legacy facts defined in the fact hash that use the top scope
# fact assignment.
if token.value.start_with?('::facts[')
fact_name = token.value.match(%r{::facts\['(.*)'\]})[1]

# This matches legacy facts defined in the fact hash.
elsif token.value.start_with?("facts['")
fact_name = token.value.match(%r{facts\['(.*)'\]})[1]

# This matches using legacy facts in a the new structured fact. For
# example this would match 'uuid' in $facts['uuid'] so it can be converted
# to facts['dmi']['product']['uuid']"
elsif token.value == 'facts'
fact_name = hash_key_for(token)

elsif token.value.start_with?("facts['")
fact_name = token.value.match(%r{facts\['(.*)'\]})[1]
# Now we can get rid of top scopes. We don't need to
# preserve it because it won't work with the new structured facts.
elsif token.value.start_with?('::')
fact_name = token.value.sub(%r{^::}, '')
end

next unless EASY_FACTS.include?(fact_name) || UNCONVERTIBLE_FACTS.include?(fact_name) || fact_name.match(Regexp.union(REGEX_FACTS))
Expand Down
33 changes: 33 additions & 0 deletions spec/unit/puppet-lint/plugins/legacy_facts/legacy_facts_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,14 @@
expect(problems).to have(1).problem
end
end

context 'top scoped fact variable using legacy facts hash variable in interpolation' do
let(:code) { "$::facts['osfamily']" }

it 'detects a single problem' do
expect(problems).to have(1).problem
end
end
end

context 'with fix enabled' do
Expand Down Expand Up @@ -165,6 +173,31 @@
end
end

context 'fact variable using top scope $::facts hash' do
let(:code) { "$::facts['os']['family']" }

it 'does not detect any problems' do
expect(problems).to have(0).problem
end
end

context "fact variable using legacy top scope $::facts['osfamily']" do
let(:code) { "$::facts['osfamily']" }
let(:msg) { "legacy fact 'osfamily'" }

it 'only detects a single problem' do
expect(problems).to have(1).problem
end

it 'fixes the problem' do
expect(problems).to contain_fixed(msg).on_line(1).in_column(1)
end

it 'uses the facts hash' do
expect(manifest).to eq("$facts['os']['family']")
Copy link
Collaborator

Choose a reason for hiding this comment

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

in the future we should consider reworking the legacy_fact/topscope_fact/topscope_variable check because they heavily overlap.

Copy link
Collaborator

Choose a reason for hiding this comment

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

would it make sense to add some tests from https://github.com/puppetlabs/puppet-lint/blob/e3815cebdfa50100acb888e29fde8f03f4e1879b/spec/unit/puppet-lint/plugins/top_scope_facts/top_scope_facts_spec.rb here? But expect no changes? I want to prevent that legacy_fact and topscope_fact fix the same code.

Copy link
Author

Choose a reason for hiding this comment

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

I was looking in to this yesterday. It would seem that they no longer clash but adding extra tests is never a bad thing.

end
end

context 'fact variable using legacy $::osfamily' do
let(:code) { '$::osfamily' }
let(:msg) { "legacy fact 'osfamily'" }
Expand Down