Skip to content

(PUP-11752) Fix fqdn_rand_string_spec.rb test #1308

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

Merged
merged 1 commit into from
Apr 21, 2023

Conversation

alexjfisher
Copy link
Collaborator

Since Puppet 7.23.0 fqdn_rand uses the modern structured networking fact instead of the legacy fqdn fact.

In this commit we mock the new fact if using 7.23.0 or later.

Rebase of #1294 incorporating @joshcooper 's suggestion.

@alexjfisher alexjfisher requested review from a team, b4ldr, bastelfreak, ekohl and smortex as code owners April 21, 2023 14:55
@CLAassistant
Copy link

CLAassistant commented Apr 21, 2023

CLA assistant check
All committers have signed the CLA.

bastelfreak
bastelfreak previously approved these changes Apr 21, 2023
Since Puppet `7.23.0` `fqdn_rand` uses the modern structured
`networking` fact instead of the legacy `fqdn` fact.

In this commit we mock the new fact if using `7.23.0` or later.

This is a rebase of
puppetlabs#1294

and incorporates Aria Li's original change and a variation on Josh
Cooper's suggestion for keeping compatibility with Puppet < `7.23.0`
Copy link
Collaborator

@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.

Would it make sense to just mock fqdn_rand, rather than the implementation detail of how it's determined?

@alexjfisher
Copy link
Collaborator Author

Would it make sense to just mock fqdn_rand, rather than the implementation detail of how it's determined?

If you want, but this issue has been open for a couple of months with no fix being merged.

https://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good

Same argument could be made for my ensure_packages fix. That's been sat there for a long time now. IMO, nothing stops either of these fixes being merged and then someone circling back later to replace with a better fix.

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

Successfully merging this pull request may close these issues.

6 participants