-
Notifications
You must be signed in to change notification settings - Fork 614
(MODULES-1394) replace validate_db_connection type with custom type #879
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
(MODULES-1394) replace validate_db_connection type with custom type #879
Conversation
This replaces the defined type `validate_db_connection` with a new custom type called `postgresql_conn_validator`. It's functionality is nearly a copy of its predecessor with the exception of the `create_db_first` parameter which is taken care of with a collector in service.pp. The old type is still intact but all docs have been removed and a warning has been attached to it announcing its deprecation.
bfbce3a
to
ac21c60
Compare
@hasegeli , it would be awesome to have your input on this |
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.
Looks good to me in general.
manifests/validate_db_connection.pp
Outdated
@@ -63,6 +65,9 @@ | |||
$env = $pass_env | |||
} | |||
|
|||
warning($validate_cmd) | |||
warning($env) | |||
|
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.
Debugging?
validate do |value| | ||
if value | ||
value =~ /[0-9]+/ | ||
end |
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.
Maybe it is better to check this as an integer.
@resource = resource | ||
end | ||
|
||
def build_psql_cmd |
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.
Postgres has the PQping()
command on its binary protocol to issue a connection check. It can be used with pg_isready
binary. I think it would make a good default provider. It would be more efficient and easier to use, because you don't need to pass authentication. It also supports a custom timeout.
cmd_parts = { | ||
:host => "-h #{@resource[:host]}", | ||
:port => "-p #{@resource[:port]}", | ||
:db_username => "-U #{@resource[:db_username]}", |
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.
Some short and some long arguments don't please my eyes. I prefer --xxx=yyy
format on scripts when possible to avoid argument parsing issues.
end | ||
|
||
def build_validate_cmd | ||
"/bin/echo 'SELECT 1' | #{parse_connect_settings.join(' ')} #{build_psql_cmd} " |
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.
I wouldn't spawn another process but use --command='SELECT 1'
.
ruby.rb
Outdated
def psql_cmd | ||
final_cmd = [] | ||
|
||
cmd_init = "#{resource[:psql_path]} --tuples-only --quiet " |
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-psqlrc
is also always a good idea on scripts. Otherwise the client's configuration can interfere.
e691732
to
8353538
Compare
Addresses a few of @hasegeli\'s comments. Adds a `command` parameter for more customizability.
8353538
to
61ad33d
Compare
(MODULES-1394) replace validate_db_connection type with custom type
This replaces the defined type
validate_db_connection
with a newcustom type called
postgresql_conn_validator
. It's functionality isnearly a copy of its predecessor with the exception of the
create_db_first
parameter which is taken care of with a collector inservice.pp. The old type is still intact but all docs have been removed
and a warning has been attached to it announcing its deprecation.