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.

Sign up for GitHub

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

Jump to bottom

Fixes #26610 - offline backup config file fix #248

Closed
wants to merge 1 commit into from
Closed

Fixes #26610 - offline backup config file fix #248

wants to merge 1 commit into from

Conversation

pjgunst
Copy link

@pjgunst pjgunst commented Mar 6, 2019

…d tar backups due to Candlepin activemq files being modified

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • 65eede7 must be in the format fixes #redmine_number - brief description
  • length of the first commit message line for 65eede7 exceeds 65 characters

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

@mbacovsky
Copy link
Member

@pjgunst thanks for the patch! The code looks good and works well. The only drawback of this solution is the the downtime required for backup is expanded with the time of backup of configs. It is about 30s on my system and I'd assume it is acceptable with regard to the time of whole backup.

This is good to merge but we have to satisfy some formal conditions.

Let me know if you need any assistance.

@mbacovsky mbacovsky mentioned this pull request Apr 2, 2019
Merged
@pjgunst
Copy link
Author

pjgunst commented Apr 15, 2019

Hi,
Redmine issue created: https://projects.theforeman.org/issues/26610

@pjgunst pjgunst changed the title scenario: backup - offline - wait for services to stop to avoid faile… Fixes #26610 - offline backup config file fix Apr 15, 2019
mbacovsky
mbacovsky reviewed Apr 15, 2019
View reviewed changes
definitions/scenarios/backup.rb
@@ -132,6 +131,7 @@ def add_offline_backup_steps
add_steps_with_context(
find_procedures(:maintenance_mode_on),
Procedures::Service::Stop,
Procedures::Backup::ConfigFiles,
Copy link
Member

Choose a reason for hiding this comment

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

Tab instead of spaces

mbacovsky
mbacovsky reviewed Apr 15, 2019
View reviewed changes
definitions/scenarios/backup.rb
@@ -162,6 +162,7 @@ def add_snapshot_backup_steps
Procedures::Backup::Snapshot::PrepareMount,
find_procedures(:maintenance_mode_on),
Procedures::Service::Stop,
Procedures::Backup::ConfigFiles,
Copy link
Member

Choose a reason for hiding this comment

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

Tab instead of spaces

@mbacovsky
Copy link
Member

@pjgunst thanks for the update!
Rubocop is not yet happy, could you please check the log in https://travis-ci.org/theforeman/foreman_maintain/jobs/520140519? You can also test locally by running rake rubocop. Removing the line from compose made the method fit the rubocop limits and we don't need the rule to be turned off. Please remove the disable comments on L22 and L41. Also you've used tab instead of spaces for indentation and our rubocop is sensitive about that (marked inline).

@mbacovsky
Copy link
Member

@pjgunst, I've amended the commit and merged manually. Thanks for your contribution!

@mbacovsky mbacovsky closed this May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbacovsky mbacovsky mbacovsky left review comments

Assignees
No one assigned
Labels
Needs re-review Needs testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pjgunst @theforeman-bot @mbacovsky