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.

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

Adding in explicit support for "watches" #24

Merged
merged 2 commits into from
Sep 24, 2014

Conversation

jrnt30
Copy link
Contributor

@jrnt30 jrnt30 commented Sep 23, 2014

Note: this resolves #23 from the puppet-consul project

concept introduced in consul 0.4.0

- Note: this resolves voxpupuli#23 from the puppet-consul
$state = undef,
$event_name = undef, #Note: this actually maps to the "name" param for event watches
) {
include consul
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this is a good idea.

I've seen this backfire in cases where a watch is defined first, and hence consul is "included" and then later declared twice.

I don't recommend doing this.

I think it would be safer to just validate that $consul::config_dir exists at the time you use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I was taking inspiration from your other two defines, the Checks and Services that used this pattern. I think I understand your suggestion and believe that is already accounted for in the bottom section, which would probably negate the need for the the explicit include. Would you like me to make similar modifications for the other two defines that are used in a similar fashion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, no it is ok. I guess my preferences on how to construct modules has changed over time.

solarkennedy added a commit that referenced this pull request Sep 24, 2014
Adding in explicit support for "watches"
@solarkennedy solarkennedy merged commit 6829d83 into voxpupuli:master Sep 24, 2014
@solarkennedy
Copy link
Contributor

Thanks! This is very solid PR. Thank you for tests!

@jrnt30
Copy link
Contributor Author

jrnt30 commented Sep 24, 2014

Glad to help! Thanks for putting it together

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

Successfully merging this pull request may close these issues.

Add support for the new "watch" resource exposed in Consul 0.4.0
2 participants