search

redhat-cop / infra.leapp

Collection of Ansible roles for automating RHEL in-place upgrades using Leapp.
MIT License
43 stars 33 forks source link

Do not unset release if release target is defined #159

Closed Denney-tech closed 3 months ago

Denney-tech commented 4 months ago

This is a proposed fix for #134.

It doesn't have any changes to docs or anything like that for the moment, but the idea is that you can set the leapp_target_release variable to some release version, and that will add it to the leapp commands for analysis/upgrade as well as preventing it from being unset in the post-upgrade steps.

Perhaps a boolean true/false would be preferred for stopping the release from being unset?

heatmiser commented 4 months ago

Reviewers, please refer to the issue here for additional background and discussion.

djdanielsson commented 4 months ago

I think we would need to test the code but from just looking at it, I think it looks good

Denney-tech commented 4 months ago

When I get a moment later today, I will add an assertion to make sure that --target isn't in the *_opts vars if leapp_target_release is also set.

That or, if preferred, we can just add a boolean variable that defaults to true for the unset task. Or we could even do both.

jeffmcutter commented 4 months ago

I think it makes sense to include a task to set it to the desired destination release for the version of RHEL upgraded to as well.

Denney-tech commented 4 months ago

@jeffmcutter do you mean having a separate subscription-manager release --set x.y task? Leapp itself sets the release to whatever it upgrades to, but I suppose there could also be a subsequent minor release target set after the major upgrade and before the minor upgrade steps.

jeffmcutter commented 4 months ago

That is what I was thinking. I'm not sure how common it would be, but with the right conditionals it would be harmless and potentially helpful.

Denney-tech commented 4 months ago

@djdanielsson @heatmiser How's this?

Denney-tech commented 4 months ago

Oh hold on, I forgot to add to README.md and add a changelog fragment.

jeffmcutter commented 3 months ago

I believe this feature is going to be required for RHEL 8 High Availability cluster to RHEL 9.

I will test using this PR.

Denney-tech commented 3 months ago

Thank you for testing, @jeffmcutter. I just resolved some merge conflicts as well.

jeffmcutter commented 3 months ago

I believe this feature is going to be required for RHEL 8 High Availability cluster to RHEL 9.

I will test using this PR.

This is not the case specifically for the HA, I misread the docs on that. Still testing.

jeffmcutter commented 3 months ago

Tested successfully. @swapdisk OK to merge?

Denney-tech commented 3 months ago

Fixed sanity checks. I had created the change fragment in browser, which created it with dos line-endings instead of unix. Should be good now.

djdanielsson commented 3 months ago

can you fix the conflict?

Denney-tech commented 3 months ago

@djdanielsson done

jeffmcutter commented 3 months ago

@djdanielsson What do we need here? https://github.com/redhat-cop/infra.leapp/pull/174? I think it would be good to get this merged.