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

Jump to bottom

(MODULES-3951) Add explicit check for stringify_facts #170

Merged
merged 3 commits into from
Oct 10, 2016
Merged

(MODULES-3951) Add explicit check for stringify_facts #170

merged 3 commits into from
Oct 10, 2016

Conversation

binford2k
Copy link
Contributor

Without this check, you get incomprehensible results. The is_pe fact is
evaluated as true, and so the class bails out. In some cases the
info() function isn't loud enough and so it appears to do nothing.

This just fails hard and makes it obvious why.

@binford2k
(MODULES-3951) Add explicit check for stringify_facts
1687132 
Without this check, you get incomprehensible results. The is_pe fact is
evaluated as true, and so the class bails out. In some cases the
info() function isn't loud enough and so it appears to do nothing.

This just fails hard and makes it obvious why.
@binford2k binford2k force-pushed the check_for_stringify branch from 7a0d5a2 to 1687132 Compare October 7, 2016 04:39
@binford2k binford2k changed the title Add explicit check for stringify_facts (MODULES-3951) Add explicit check for stringify_facts Oct 7, 2016
@glennsarti
Copy link
Contributor

Would be nice to get a spec test for this.

@puppetcla
Copy link

CLA signed by all contributors.

@binford2k
Copy link
Contributor Author

@glennsarti I'd be happy to, but I couldn't think of a non-trivial test, and I haven't spent time figuring out how to set up a beaker test with config settings. :-/

@glennsarti
Copy link
Contributor

A simple spec test should be sufficient. Mock the facts and expect a fail.

@glennsarti
(MODULES-3951) Add unit test for stringify_facts fact
251e8ef 
Previously there were no tests for the stringify_facts fact.  This commit
adds unit tests for this fact by mocking the Puppet.settings values.  This
commit also adds a test to ensure that catalog compilation fails if
stringify_facts evaluates as true.
branan
branan suggested changes Oct 10, 2016
View reviewed changes
Copy link
Contributor

@branan branan left a comment

Choose a reason for hiding this comment

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

This was already done (albeit less-cleanly) here: https://github.com/puppetlabs/puppetlabs-puppet_agent/blob/master/manifests/prepare/ssl.pp#L48-L50

This PR should probably also remove that check, since stringify_facts will now explicitly be checked

@glennsarti
Copy link
Contributor

Is it possible for that code path to be triggered by something other than stringify = true?

@branan
Copy link
Contributor

branan commented Oct 10, 2016

@glennsarti Only if a user also has their own custom fact called puppet_sslpaths that isn't a hash, or if pluginsync has somehow failed - both cases that I'm comfortable not worrying too much about.

6fb63a0 is the actual commit - the entire case is just to check if the fact is a hash.

@glennsarti
Copy link
Contributor

Excellent...I can revert those. Thanks

@glennsarti
Revert "(MODULES-3591) Test if stringify_facts = true on agent"
11fae83 
Commit 6fb63a0 was added to detect when stringify_facts is true however this
check is now superseded by the `fail` in MODULES-3951
This reverts commit 6fb63a0.
@glennsarti
Copy link
Contributor

@branan travis has completed with the changes you requested. Can you please review once more and approve once you're happy. Thanks.

branan
branan approved these changes Oct 10, 2016
View reviewed changes
Copy link
Contributor

@branan branan left a comment

Choose a reason for hiding this comment

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

All looks good now! Thanks for taking care of that

@glennsarti glennsarti merged commit 676076d into puppetlabs:master Oct 10, 2016
@puppetcla
Copy link

CLA signed by all contributors.

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

@branan branan branan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@binford2k @glennsarti @puppetcla @branan