-
Notifications
You must be signed in to change notification settings - Fork 71
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
Fixes #22945 - show failure if DB service stopped #160
Conversation
0db7803
to
36737e2
Compare
@@ -11,7 +11,7 @@ class ValidateDb < ForemanMaintain::Check | |||
|
|||
def run | |||
result, result_msg = feature(:candlepin_database).execute_cpdb_validate_cmd | |||
next_steps = [] | |||
next_steps = [Procedures::StartService.new(:service => 'postgresql')] |
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.
@iNecas,
Instead of hard-coding service name, should I need to handle this based on DB type, right?
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 guess, we can add this step later on after PR-151 . This step i.e procedure helps to start respective DB service.
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.
👍 for deferring this decision for now.
36737e2
to
c7f8d64
Compare
Needs testing but 👍 on the approach |
|
||
def up?(config = configuration) | ||
query = 'SELECT 1 as ping' | ||
execute?("PGPASSWORD='#{config[%(password)]}' #{psql_db_connection_str(config)}", |
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 duplicate code is IMO worth extracting to some private method such asrun_psql
, psql_command
or something like that, and use it bot from here and from the psql
method
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.
Also, what's the difference between this method and the ping
one? Should the ping
method be just replaced with this one or the other way around?
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.
Yes, I agree.
I can see that ping
method is not used anywhere so worth
modifying existing method & remove duplicate code. I will update PR.
c7f8d64
to
4c34fbe
Compare
@iNecas, updated PR as per suggested changes. |
Looks good...I see issue no longer exists 👍 |
end | ||
|
||
def ping(config = configuration) | ||
psql('SELECT 1 as ping', config) | ||
execute?(psql_connection_string(config), :stdin => 'SELECT 1 as ping') |
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.
One last nitpick: connection_string
phrase is connected to a special case (or at least for me, and I can imaging others having similar connotation) of passing to some driver to connect to db. In this case, I would rather go for psql_command_string
or simply just psql_command
One last comment and good to go |
4c34fbe
to
0544e16
Compare
0544e16
to
28654d6
Compare
@iNecas, |
Thanks @kgaikwad and @ntkathole |
@iNecas ,
Doing testing on my end. I will update you once done.
While testing found that foreman-task check for non-paused is also giving error -
so moved method to base-database concern.
If you have any suggestions on code please let me know.