-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add option to skip the update/reboot on pre-upgraded server #227
Conversation
@@ -29,6 +29,7 @@ | |||
|
|||
- name: leapp-upgrade | Include update-and-reboot.yml | |||
ansible.builtin.include_tasks: update-and-reboot.yml | |||
when: skip_pre_upgrade_update_reboot is not defined or not skip_pre_upgrade_update_reboot|bool |
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.
When would the var not be defined?
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.
Seems reasonable to me otherwise. I would want to remove the check for defined since it's in the defaults.
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.
Hi @jeffmcutter
Thanks for the feedback. You're right that it's in the defaults so the check should not be needed. Unfortunately, in the past I've known users to update individual task files from git rather than pulling all the updates (yes, that's bad and not logical...but that's how I've seen it done). So someone just pulling the updated task file wouldn't have skip_pre_upgrade_update_reboot
defined. I agree that's bad practice, so I'll follow your advice and remove that check.
Before I do, there is another option:
Rather than defining skip_pre_upgrade_update_reboot
I could write the logic the other way around and instead have something likepre_upgrade_update
with a default of true. Right now you have post_upgrade_update
so maybe a pre_upgrade_update
fits better?
Happy with either option as both achieves the same result.
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.
Hi @rjo-uk,
That sounds good. Consistent is always good, plus I think it will make the logic more straight forward.
Thanks,
-Jeff
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 like the pre_upgrade_update
var name.
LGTM
Changed from draft to final if you're happy with it. |
As part of the upgrade role, an initial update and reboot is performed in https://github.com/redhat-cop/infra.leapp/blob/main/roles/upgrade/tasks/leapp-upgrade.yml
Whilst this is good practice, it would be useful to have the option of disabling this so that:
This initial draft is intended to not change workflows for existing users. If the variable
skip_pre_upgrade_update_reboot
is not provided, the update and reboot will occur as it does today.