-
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
[JBEAP-27509] extending history information #720
Conversation
e738ecd
to
055bf95
Compare
17e2283
to
638bedc
Compare
@TomasHofman could you review this when you have a moment? |
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) return true; | ||
if (o == null || getClass() != o.getClass()) return false; | ||
SavedState that = (SavedState) o; | ||
return Objects.equals(hash, that.hash) && Objects.equals(timestamp, that.timestamp) && type == that.type; | ||
return Objects.equals(manifestVersions, that.manifestVersions) && Objects.equals(hash, that.hash) && Objects.equals(timestamp, that.timestamp) && type == that.type && Objects.equals(msg, that.msg); |
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.
It's probably not very important, but since the manifest versions are involved in the equals method, should the collection type for versions be Set, rather than List?
(Thinking purely about ordering, which I assume is generally undefined - version collections should be equal if they contain the same elements, irrespective of order. I think Set would facilitate that.)
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.
Good point, I'm going to sort the manifestVersions
in the constructor. This way the ordering of the versions will be consistent in display.
public Version(String identifier, String mavenVersion, String logicalVersion) { | ||
this.identifier = identifier; | ||
this.physicalVersion = mavenVersion; | ||
this.logicalVersion = logicalVersion; | ||
} |
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.
This would probably deserve a javadoc about what the logical and physical versions mean.
To double check if this was intended, when I run the new history command against an old installation, I think it delegates to the old impl and shows something like this:
It shows "install [G:A::version]" at the first record. Running the command against a newly created installation, it doesn't show the GA anymore, just the version:
|
High level question: since the original impl of the history command also shows versions of manifests, what is the intended improvement here? |
Thanks @TomasHofman I updated the description to explain the changes. Let me know if that clarifies it.
|
638bedc
to
12a3916
Compare
OK, thanks for the explanation, I noticed the new schema version by accident but didn't know the details. Now it makes sense to me 👍 . |
Thanks @TomasHofman 👍 |
Add current manifest versions to the git commit so that they can be displayed in history operations
== Overview
The channel manifest 1.1.0 adds a notion of
logicalVersion
to separate the version of manifest from the Maven coordinate. This change will consume the new field falling back to the Maven coordinate (physical version) if the logicalVersion is not present.Additionally, the PR changes how the manifest version information is stored. Previously the version string visible in history information was constructed during update and stored. This make it hard for API clients that would like to display the versions of individual manifests.
== Expected results