-
Notifications
You must be signed in to change notification settings - Fork 580
(MODULES-1982)Create a tcp_conn_validator resource #444
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
Conversation
|
||
module Puppet | ||
module Util | ||
class TcpValidator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason you're putting this into the puppet::util namespace, rather than just creating our own?
it kinda feels… dirty…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No special reason. I suspected it was a pattern because both puppetlabs-mongodb[1] and puppetlabs-puppetdb[2] uses it.
[1] https://github.com/puppetlabs/puppetlabs-mongodb/blob/master/lib/puppet/util/mongodb_validator.rb
[2] https://github.com/puppetlabs/puppetlabs-puppetdb/blob/master/lib/puppet/util/puppetdb_validator.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to move that to lib/puppetx/puppetlabs, as per https://github.com/puppetlabs/puppet/blob/master/lib/puppet_x.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DavidS so you want this function to be in PuppetX::Util::TcpValidator scope
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The puppet_x.rb suggests to use puppetx/moduleowner
as namespace. PuppetX::Puppetlabs::TcpValidator
would sound good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, that should have been puppet_x/moduleowner
d6c326d
to
876340c
Compare
@DavidS PR updated to reflect the namespace change. Is that ok? Should the path to the TcpValidator class should be change or is it fine as it is? |
Uhm, yeah, that would be great. File names should almost always match their contents. |
876340c
to
cc16fee
Compare
@DavidS done. |
Some services might take a while to start, even though the service returned the boot process is finish, the process is not yet actually listening on the port. This resource would allow to ensure a service is really up and running before continuing with the application of the catalog. Other project already rely on a similar feature. [1][2] [1] https://github.com/puppetlabs/puppetlabs-mongodb/blob/master/lib/puppet/type/mongodb_conn_validator.rb [2] https://github.com/puppetlabs/puppetlabs-puppetdb/blob/master/lib/puppet/type/puppetdb_conn_validator.rb
cc16fee
to
07beae4
Compare
We're thinking a new module for this, discussion: https://groups.google.com/forum/#!topic/puppet-dev/5hZcxDk6pjk |
prevent configuration changes from being applied if the server cannot be | ||
reached, but it could potentially be used for other purposes such as monitoring." | ||
|
||
ensurable do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this resource doesn't really have an actual "state" I don't think it should be ensurable - having something like a "reachable => true/false" seems like a better approach to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would reachable => false, mean 'make sure this thing is down before proceeding?'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, either those are the semantics or remove this sort of "ensurable" or "reachable" parameter entirely and indicate that this check must always pass. As it stands, I think that this resource will fail regardless of if it's present or absent.
Can you submit this to puppetcommunity/puppet-healthcheck? |
@nibalizer will do. Will catch up with the thread in the ML also. |
|
The result of the test was: PASS I am a beta ci bot. I am probably lying to you. |
The result of the test was: PASS I am a beta ci bot. I am probably lying to you. |
As asked by @nibalizer and announced in the thread on puppet-dev this PR has been made against puppet-community/puppet-healtcheck module voxpupuli/puppet-healthcheck#1 Closing it here. |
\o/ |
Some services might take a while to start, even though the service
returned the boot process is finish, the process is not yet actually
listening on the port. This resource would allow to ensure a service is
really up and running before continuing with the application of the
catalog. Other project already rely on a similar feature. [1][2]
[1]
https://github.com/puppetlabs/puppetlabs-mongodb/blob/master/lib/puppet/type/mongodb_conn_validator.rb
[2]
https://github.com/puppetlabs/puppetlabs-puppetdb/blob/master/lib/puppet/type/puppetdb_conn_validator.rb