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

Avoid upgrading from v5.3.1 on Windows #25135

Merged

Conversation

l0rd
Copy link
Member

@l0rd l0rd commented Jan 27, 2025

This PR adds a condition in the Windows WiX bundle that prevents upgrades from v5.3.1 and recommends the user to upgrade to v5.3.2 first.

Version 5.3.1 of the installer had a bug that got patched in v5.3.x. The patch won't be included in the installer for v5.4.0 (see here for details), and we can't allow direct upgrades from v5.3.1 to v5.4.x anymore.

Does this PR introduce a user-facing change?

None

Added a condition in the Windows WiX bundle that
prevents upgrades from v5.3.1 and recommend the
user to upgrade to v5.3.2 first.

That's needed because version 5.3.1 of the installer
had a bug that got patched in v5.3.2 only.

c.f. containers#24735

Signed-off-by: Mario Loriedo <[email protected]>
@openshift-ci openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 27, 2025
@l0rd l0rd marked this pull request as ready for review January 27, 2025 14:57
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 27, 2025
@l0rd l0rd added the v5.4 label Jan 27, 2025
@mheon
Copy link
Member

mheon commented Jan 27, 2025

Timeout on one of the applehv tests with the new 45 minute timeout.
Restarted

@mheon
Copy link
Member

mheon commented Jan 27, 2025

LGTM

@l0rd
Copy link
Member Author

l0rd commented Jan 27, 2025

all tests are good @containers/podman-maintainers PTAL

@Luap99
Copy link
Member

Luap99 commented Jan 28, 2025

I cannot really review any of the changes.
The obvious question is why this needs to be done, it is not obvious from the commit message because the real question then is why do we not have the patch in 5.4? Looks like the answer is in #25021 but given that was not linked here it was hard to follow the reason

# with
# `Get-Latest-Podman-Setup-From-GitHub`
$env:PREV_SETUP_EXE_PATH = Get-Podman-Setup-From-GitHub -version "tags/v5.3.0"
$env:PREV_SETUP_EXE_PATH = Get-Latest-Podman-Setup-From-GitHub
Copy link
Member

Choose a reason for hiding this comment

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

I still like to insist that is is fundamentally flawed approach. We should NEVER depend on rolling targets as this starts breaking on random PRs and block any development in the meantime. #24653

The proper approach should be to hard docs the version and then update said version after each release. Yes annoying but that can be automated with renovate.

The reason why this is so important is release branches, if we always pull the latest here it means a release branch, i.e. old 4.9 branch that was used for a bit, will then pull the latest and that might fail the installer update testing because the code there is much older. Now with our current releases and just supporting the latest release there will not be often cases where this might play a role but it can bite us in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are protected against problems like #24653 because we are now testing upgrades from the main branch (in addition to testing upgrades from the release branch). This allows us to detect problems much earlier.

You are right in general, but I am trying to be pragmatic. We aren't particularly good at updating versions in Windows-specific code.

Copy link
Member

Choose a reason for hiding this comment

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

We aren't particularly good at updating versions

Right which is why my advocating for automating these via renovate.

Or maybe do not hard code the same version in many places and use something like
grep github.com/containers/gvisor-tap-vsock go.mod | cut -d" " -f2 or however that would be done in powershell.

But yeah I am fine to block on on that, we clearly want this in 5.4 so I am fine with doing this for now.
But I Really think long term this is not sustainable

@l0rd
Copy link
Member Author

l0rd commented Jan 28, 2025

I cannot really review any of the changes. The obvious question is why this needs to be done, it is not obvious from the commit message because the real question then is why do we not have the patch in 5.4? Looks like the answer is in #25021 but given that was not linked here it was hard to follow the reason

@Luap99 thank you for the review and I have updated the description. Your guess is correct. This is a follow-up to #25021 to avoid a regression of #24735 for someone who tries to upgrade from v5.3.1 to 5.4.x.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

confirmed the new test cases seem to be running per CI logs. As I don't anyone else from us can properly review/understand the installer bits either I go ahead and merge it

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2025
Copy link
Contributor

openshift-ci bot commented Jan 28, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: l0rd, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 92bce4f into containers:main Jan 28, 2025
88 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note-none v5.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants