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

Update process should check version and not allow reversion through update #765

Closed
wants to merge 1 commit into from
Closed

Update process should check version and not allow reversion through update #765

wants to merge 1 commit into from

Conversation

parsharma
Copy link
Contributor

Issue: #759

@parsharma parsharma changed the title Draft:Update process should check version and not allow reversion through update Update process should check version and not allow reversion through update Sep 23, 2024
@TomasHofman
Copy link
Contributor

Relevant wildfly-channel issue that I think we would need implemented there in order to complete this functionality:
wildfly-extras/wildfly-channel#292

@TomasHofman
Copy link
Contributor

A good next step could be to obtain a list of manifests used by the current installation. I think it should be possible to obtain maven GAVs (or URLs) of the current manifests. If there are any Maven based manifest found in the list, they should be matched against the list of new manifests (which the user want to upgrade to) used by the ChannelSession.

(That list of new manifests is currently simulated by the findManifestUpdates() method, hard-coded to only return org.jbosss.eap.channels:eap-8.0 manifest artifact, because we don't know how to obtain these data.)

@parsharma
Copy link
Contributor Author

Hello @TomasHofman,
I have updated the code where we are getting list of manifests from the current installation.

@parsharma parsharma force-pushed the 759 branch 2 times, most recently from 62b52fb to 955a431 Compare September 25, 2024 13:52
@TomasHofman
Copy link
Contributor

Now can we get the list of manifests currently used by the installation? Maybe from the InstallationMetadata class?

If we can have that, we can try to obtain an intersection from those two lists (manifests groupId:artifactId s that are present in both currently installed manifest and the newly available manifests used by the channelSession).

Once we have that intersection, we can try to compare the versions to see if they are upgrading or downgrading.

@parsharma
Copy link
Contributor Author

@TomasHofman
I have added the new method compareManifest() where this method compares the currently installed Maven manifests with the available manifest updates from the ChannelSession. It finds the manifests that exist in both the installed manifests and the available updates (based on groupId and artifactId). For each matching manifest, it compares the version of the installed manifest with the version from the available update. Depending on the comparison, the method outputs whether an upgrade, downgrade, or no change is required.

TomasHofman
TomasHofman reviewed Oct 3, 2024
View reviewed changes
prospero-cli/src/main/java/org/wildfly/prospero/cli/commands/UpdateCommand.java Outdated
Comment on lines 510 to 531
if (updateArtifact != null) {
// Compare versions
String installedVersion = installedManifest.getVersion();
String availableVersion = updateArtifact.getVersion();

if (installedVersion.compareTo(availableVersion) < 0) {
System.out.println("Upgrade available for " + installedManifest.getArtifactId() + ": " + installedVersion + " -> " + availableVersion);
} else if (installedVersion.compareTo(availableVersion) > 0) {
System.out.println("Downgrade detected for " + installedManifest.getArtifactId() + ": " + installedVersion + " -> " + availableVersion);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the comparison logic here should not be an alphabetic comparison, but something that understands semantic versioning.

The problem is that e.g. version "1.11" would be evaluated as lower than "1.2" when using alphabetical ordering, while it should be higher semantically.

Copy link
Contributor Author

@parsharma parsharma Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for noticing it @TomasHofman
I have updated the code where I am using DefaultArtifactVersion where we compare the versions semantically.

Copy link
Collaborator

Choose a reason for hiding this comment

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

org.wildfly.channel.version.VersionMatcher has the correct version compare logic

spyrkob
spyrkob reviewed Oct 3, 2024
View reviewed changes
prospero-cli/src/main/java/org/wildfly/prospero/cli/commands/UpdateCommand.java Outdated
}

// For each intersecting manifest, compare the version
for (ManifestVersionRecord.MavenManifest installedManifest : intersectingManifests) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't need the intersectingManifest list. Just iterate over mavenManifests. You're already ignoring ones that are not in updateArtifacts.

@parsharma parsharma force-pushed the 759 branch 2 times, most recently from 5c837cd to 83e7704 Compare October 9, 2024 06:39
@parsharma
Copy link
Contributor Author

The CI/CD is failing because we need wildfly-channel upgrade for the API.

@parsharma parsharma marked this pull request as ready for review October 16, 2024 11:28
@TomasHofman
Copy link
Contributor

Yes, we must ask Jeff to release.

@TomasHofman
Copy link
Contributor

Never mind, he did and you upgraded already.

@parsharma parsharma force-pushed the 759 branch 2 times, most recently from 6fb4887 to 3af3f0e Compare October 18, 2024 08:03
@TomasHofman
Copy link
Contributor

@spyrkob currently this only checks for downgrade in the update perform command. There is the second pathway with update prepare / update apply. Should the check be made during prepare or apply stage? If I prepare and update, and then the installation changes, is the prepared update still applicable or do I need to prepare it again?

@TomasHofman
Copy link
Contributor

Maybe we could move the check into UpdateCommand#buildUpdate() method, which would perform the check both during update perform and update prepare.

@spyrkob
Copy link
Collaborator

spyrkob commented Oct 18, 2024

I prepare and update, and then the installation changes, is the prepared update still applicable or do I need to prepare it again?

If the base server changes from the state that the candidate was generated from, that candidate is no longer valid and cannot be applied.

@parsharma parsharma force-pushed the 759 branch 3 times, most recently from 1921899 to 9e0511f Compare October 18, 2024 10:59
@spyrkob
Copy link
Collaborator

spyrkob commented Nov 1, 2024

Thanks @parsharma I added a few things to your changes in #784. I'll close this PR and merge the new one

@spyrkob spyrkob closed this Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spyrkob spyrkob spyrkob left review comments

@TomasHofman TomasHofman TomasHofman left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@parsharma @TomasHofman @spyrkob