Skip to content

allows for 0 _ttl's without time signifier and enables tests#50

Merged
kbarber merged 1 commit into
puppetlabs:masterfrom
ghoneycutt:ttl_checking
Apr 9, 2013
Merged

allows for 0 _ttl's without time signifier and enables tests#50
kbarber merged 1 commit into
puppetlabs:masterfrom
ghoneycutt:ttl_checking

Conversation

@ghoneycutt

Copy link
Copy Markdown
Contributor

This commit allows you to specify '0' for a _ttl param without having to specify the time signifier. It then validates the _ttl params within the manifest.

Also adds support for spec testing and tests these cases.

@kbarber kbarber merged commit fc3f8dc into puppetlabs:master Apr 9, 2013
Comment thread spec/classes/init_spec.rb

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't really test what you want to test, I'm removing it for now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@kbarber

Lines 4-16 should be testing that if you just 'include puppetdb' and use all the defaults, that it should contain the puppetdb class, ie: not fail.

Is that the code to which you are referring?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now - I mean the block referenced by line 18.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, that part need the :params hash set to use '0' for the ttl_args

The next part on line 23 should have the :params hash to set something like 1S (capital) which should still work since the code uses downcase()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep - you'll need a new PR for that though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the insight and help.

@kbarber

kbarber commented Apr 9, 2013

Copy link
Copy Markdown
Contributor

I merged this in manually with some cleanup, I removed one of the tests because it doesn't do anything - take a look at my comment. Other than that, thanks @ghoneycutt.

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.

3 participants