-
Notifications
You must be signed in to change notification settings - Fork 24
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
Persist manifest name and expose it via SPI #481
Conversation
6d0717d
to
1079a9d
Compare
5325eae
to
3d97b8b
Compare
3d97b8b
to
0d0f6ba
Compare
public MavenManifest(@JsonProperty("groupId") String groupId, | ||
@JsonProperty("artifactId") String artifactId, | ||
@JsonProperty("version") String version) { | ||
@JsonProperty("version") String version, | ||
@JsonProperty(value = "description", required = false) String description) { |
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.
Should that be called "name", rather then "description"? It looks to be equivalent with manifest "name" from the wildfly-channel lib, while there is also "description" in the wildfly-channel lib that would be easy to confuse with this field.
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 wanted to separate where the data is coming from in the manifest YAML from the field in record YAML, in case we decide to use a different manifest in the future. But yeah that's a bit confusing, I don't mind changing that to "name"
if (latestVersion.isPresent()) { | ||
final String description = getManifestDescription(new ChannelManifestCoordinate( | ||
manifestCoordinate.getGroupId(), manifestCoordinate.getArtifactId(), latestVersion.get() | ||
), mavenVersionsResolver); | ||
manifestVersionRecord.addManifest(new ManifestVersionRecord.MavenManifest( | ||
manifestCoordinate.getGroupId(), | ||
manifestCoordinate.getArtifactId(), | ||
latestVersion.get(), | ||
description)); | ||
} else { |
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.
Maybe I read this wrongly; is this method supposed to return versions of manifests currently used by installation? But this branch in fact returns latest available version which may not be used by given installation? Shouldn't we rather say we don't know the version in this case?
Also to clarify when these cases realistically happen, if I provision installation with just manifest G:A (without specifying version), the version would be present in these data or not?
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.
In this case the version is resolved from an offline repository which returns it from already resolved maven cache producing the last used version of the manifest.
Yes the record will show the version of the resolved manifest
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.
Does that offline repository contain multiple manifest versions at once (after running some upgrades), or always only the single version that reflects current state?
So if I rollback installation into a previous state using the history command, it is possible that this shows a wrong version then?
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.
That's good point, yes in case of a revert we cannot rely on the version in repository. When reverting a server, the version record from the previous state is used instead.
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.
Looks good to me.
requires wildfly-extras/wildfly-installation-manager-api#18