Skip to content

Conversation

bastelfreak
Copy link
Member

Followup for #53 (review)

@bastelfreak bastelfreak added the Bug Something isn't working label Dec 16, 2024
@bastelfreak bastelfreak self-assigned this Dec 16, 2024
@bastelfreak
Copy link
Member Author

(I haven't tested this locally)

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
@ekohl
Copy link
Member

ekohl commented Dec 17, 2024

I'm guessing test failures are related

@bastelfreak
Copy link
Member Author

I'm not 100% sure where spec/static_fixtures/fake.host.fqdn.com.yaml is coming from. It's a Puppet::Node::Facts object. Is that the kind of facts the puppetserver stored a decade ago? On modern systems you just have /opt/puppetlabs/server/data/puppetserver/server_data/facts/*.json, which is the same as puppet facts show, no Ruby object.

@ekohl
Copy link
Member

ekohl commented Dec 17, 2024

There is still code to load data from nodes so perhaps it's worth digging into when that was removed:

# if there is no environment in facts
# get it from node file ({puppetdir}/yaml/node/
unless puppet_facts['values'].key?('environment') || puppet_facts['values'].key?('agent_specified_environment')
node_filename = filename.sub('/facts/', '/node/')
if File.exist?(node_filename)
node_data = parse_file(node_filename)
if node_data.key?('environment')
puppet_facts['values']['environment'] = node_data['environment']
end
end
end

@bastelfreak
Copy link
Member Author

https://www.puppet.com/docs/pe/2023.0/osp/server/release_notes.html#puppet-server-700

puppetserver 7 switched the default facts terminus from yaml to json.

@alexjfisher
Copy link

Because there's still a fallback to certname I think this should be safe. But for any similar changes, don't automatically assume all facts exist. What comes to mind, is puppet device. The facts returned by the providers random network devices use aren't the full set you'd usually expect from a normal agent.

@alexjfisher
Copy link

Because there's still a fallback to certname I think this should be safe. But for any similar changes, don't automatically assume all facts exist. What comes to mind, is puppet device. The facts returned by the providers random network devices use aren't the full set you'd usually expect from a normal agent.

or maybe it could still be an issue??? I can't really remember where how certname was set when using Foreman with some puppet device things. Maybe I was using the --push-facts code path here?? I really can't remember. I guess the safest change would be to use puppet_facts.dig('values', 'networking', 'fqdn') || puppet_facts['values']['fqdn'] || certname?

See https://github.com/f5devcentral/f5-puppet/blob/defaf02856282ddea61b81bdcc9473f34ffff812/lib/puppet/util/network_device/f5/facts.rb#L14 for an example of how limited facts from a 'device' can be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants