fixes #18328, #20268 - adds interaction to confirm service shutdown, provides warning for online backup

Katello / katello-packaging

[DEPRECATED] Packaging for Katello

7 stars 33 forks source link

fixes #18328, #20268 - adds interaction to confirm service shutdown, provides warning for online backup #459

Closed cfouant closed 7 years ago

cfouant commented 7 years ago

This adds interaction to confirm before stopping services, or proceed with online backup after reading warning. It also adds a "-y/--assumeyes" flag to bypass interaction by assuming yes to questions.

johnpmitsch commented 7 years ago

Should we make it default to confirm the stopping of services? There could be a -y/--yes flag to override. I'm not sure which tactic would be better for users.

cfouant commented 7 years ago

@johnpmitsch that would likely break a lot of cron jobs, so we are going this route instead

johnpmitsch commented 7 years ago

@cfouant I am thinking we should go the route of shutting down the services by default and then the user can override with -y for scripting. From the issue:

This can have serious consequences if the user does not know ahead of time that the server will be shut down during the backup.

if the user doesn't know that services are going to be shut down, will they really pass a--confirm-shutdown flag? To me, it seems like to address the original intent of this issue and user frustration we should default to a confirmation and have a flag to override (similar to a command like yum install).

There is also the benefit of the user taking responsibility for their actions - if they pass with a flag to override the confirmation and there are negative consequences, that is because of their specific intent to bypass a check, not because they didn't know of the action happening. (aka not our fault! :) )

cfouant commented 7 years ago

@johnpmitsch I agree, it seems counterintuitive to add this flag, and doesn't really make a ton of sense. When I first was planning this @ehelms pointed out doing so would break existing cron scripts. @ehelms - your thoughts? @johnpmitsch makes a good point

cfouant commented 7 years ago

@johnpmitsch please re-review, PR has been updated to by default get user input before shutting down services.

johnpmitsch commented 7 years ago

@cfouant approved, you need a rebase and then it's good to go :)

theforeman-bot commented 7 years ago

There were the following issues with the commit message:

length of the first commit message line for d4d9acd88bbdba5b015b3515ea15d52ef05cbf77 exceeds 65 characters
commit message for d4d9acd88bbdba5b015b3515ea15d52ef05cbf77 is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

theforeman-bot commented 7 years ago

There were the following issues with the commit message:

length of the first commit message line for 9db2f39824a5ccf6041473162a54c1555f501577 exceeds 65 characters
commit message for 9db2f39824a5ccf6041473162a54c1555f501577 is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

cfouant commented 7 years ago

@johnpmitsch updated!