Skip to content
This repository has been archived by the owner on Feb 4, 2020. It is now read-only.

Fix some issues with 3.7.* Puppet #31

Merged
merged 2 commits into from
Jan 2, 2015

Conversation

tayzlor
Copy link

@tayzlor tayzlor commented Dec 23, 2014

Initial PR - Will see what comes out the back of the test result

@solarkennedy
Copy link
Contributor

Tests seem fine. Is there a particular variable that was not getting into the template that required you to use inherit? If so, can you write a test to ensure that variable is in the template for the next time? (so it won't break in the future)

$user = ''
$pass = ''
$refresh = 5
$refresh = '5'
Copy link

Choose a reason for hiding this comment

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

On one hand, converting to strings allows validate_string to work, but it might be unintuitive to a user. I would personally have not quoted these values if I had to specify them.

Instead of turning these values into strings, they can be left as parameters and instead of using validate_re, something like the following can be done:

  if is_integer($port) == false {
    fail("The port parameter must be an integer. The current value is ${port}.")
  }

  if is_integer($refresh) == false {
    fail("The refresh parameter must be an integer. The current value is ${refresh}.")
  }

Or to make everyone happy, something like:

  if is_integer($port) == false or $port =~ /\d+/ {
    fail("The port parameter must be an integer. The current value is ${port}.")
  }

  if is_integer($refresh) == false or $refresh =~ /\d+/ {
    fail("The refresh parameter must be an integer. The current value is ${refresh}.")
  }

(I haven't verified that last example works)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this is confusing, but in many cases (non-hiera) the thing ends up being a string, so I do think we (Yelp) would needs the second everyone-happy case.

@jtopjian
Copy link

jtopjian commented Jan 2, 2015

I ran into this today as well and was about to submit a pull request until I saw an existing one. My solution was different and described in the comments.

solarkennedy added a commit that referenced this pull request Jan 2, 2015
Fix some issues with 3.7.* Puppet
@solarkennedy solarkennedy merged commit e48e3f0 into Yelp:master Jan 2, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants