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.

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

Do not unset release if release target is defined #159

Merged
merged 13 commits into from
Apr 15, 2024
Merged

Do not unset release if release target is defined #159

merged 13 commits into from
Apr 15, 2024

Conversation

Denney-tech
Copy link
Contributor

@Denney-tech Denney-tech commented Mar 12, 2024

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
Copy link
Collaborator

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

@djdanielsson
Copy link
Collaborator

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

@Denney-tech
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

@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
Copy link
Collaborator

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 Denney-tech marked this pull request as ready for review March 15, 2024 15:27
@Denney-tech
Copy link
Contributor Author

@djdanielsson @heatmiser How's this?

@Denney-tech
Copy link
Contributor Author

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

@jeffmcutter
Copy link
Collaborator

jeffmcutter commented Mar 21, 2024

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
Copy link
Contributor Author

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

@jeffmcutter
Copy link
Collaborator

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
Copy link
Collaborator

Tested successfully. @swapdisk OK to merge?

@Denney-tech
Copy link
Contributor Author

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
Copy link
Collaborator

can you fix the conflict?

@Denney-tech
Copy link
Contributor Author

@djdanielsson done

@jeffmcutter
Copy link
Collaborator

@djdanielsson What do we need here? #174? I think it would be good to get this merged.

Copy link
Collaborator

@djdanielsson djdanielsson left a comment

Choose a reason for hiding this comment

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

LGTM

@djdanielsson djdanielsson merged commit 747ae09 into redhat-cop:main Apr 15, 2024
23 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants