-
Notifications
You must be signed in to change notification settings - Fork 39
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
Versioning #198
Versioning #198
Conversation
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, I've suggested some minor changes. Thanks @ahgittin
guide/blueprints/catalog/bundle.md
Outdated
following Brooklyn's [versioning](versioning.html) rules. | ||
Brooklyn will keep track of that bundle, allow SNAPSHOT-version bundles to be replaced, | ||
and ensure dependent bundles (specified as `brooklyn.libraries` or, for people familiar | ||
with OSGi, the `Required-bundle` manifest header) are available for searching. |
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 would probably remove for searching
as it's not clear what searching is in this context.
For example, `1.0`. `1.0.1` or `1.0.1-20150101`. | ||
In order to do this, Brooklyn requires that the `id:version` string be unique across the catalog: | ||
it is normally an error to add a type if a type with the same `id:version` is present. | ||
Exceptions are if the definition is identical, or if the `version` is noted as a `SNAPSHOT`. |
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.
Exceptions are -> The exceptions to this are
hence the bundle replacement noted above.) | ||
|
||
If you are creating an OSGi `MANIFEST.MF` for a bundle that also contains a `catalog.bom`, | ||
you will need to use the mapped rsult (OSGi version syntax) in the 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.
rsult -> result
Thanks @drigodwin, all addressed. Should not be merged until after apache/brooklyn-server#743 . Also added explanation about the |
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.
One concern about the version comparison.
* Use a `-SNAPSHOT` qualifer suffix on your version when developing | ||
* Increase the version number when making a change to a non-SNAPSHOT type | ||
|
||
When adding to the catalog, if no version is supplied, Brooklyn may automatically |
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.
Not keen on the "may" here, it seems too woolly - will Brooklyn increment the version or won't it, under what circumstances? Similarly below.
the highest ordered version (see "Ordering" below) in the catalog for the referenced type, | ||
and will thereafter lock the use of that version in that blueprint. | ||
(An exception is where types are co-bundled or an explicit search path is defined; | ||
in the context of evaluating one type, Brooklyn may prefer versions in the same bundle or on the search path.) |
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.
"may" again
The natural order here is defined as ASCII-lexicographic comparison | ||
for any non-numeric segments (`"a" < "b"`) but numeric order for digit sequences | ||
(`"9" < "10"`), so it does what is usually expected for versions (`1.9` < `1.10` | ||
and `"1.1-rc9-b" < "1.1-rc10-a"`). |
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 actually puts us at odds with OSGI - they define qualifier comparison to be simply lexicographic. For OSGI, 1.1.0.rc9-b > 1.1.0.rc10-a
. If we are converting versions to bundle versions internally we need to match the OSGI version comparison rules. E.g. when brooklyn is working out what the latest version of a bundle is, we don't want Brooklyn choosing a different version than OSGI thinks is the latest.
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.
Will make this difference explicit -- as noted in apache/brooklyn-server#740 I think this is the right thing to do.
you will need to use the mapped result (OSGi version syntax) in the manifest, | ||
but should continue to use the Brooklyn-recommended syntax in the `catalog.bom`. | ||
|
||
For those who are curious, the reason for the Brooklyn version syntax is to reconcile |
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.
👍 a great explanation to have
Thanks for great review comments. All but one incorporated, including discussion about equality @geomacy . The one that isn't is the statement about it "may" use co-bundling for searching. This is because we want to get there but currently aren't, or mostly aren't, so being precise here is hard and might become stale. Hopefully we'll soon always do it and can update that word in the docs! |
As apache/brooklyn-server#743 is merged I'm merging this. |
docs re cleared-up OSGi/semver/maven versioning, as per ML
(this is WIP because the corresponding code is not yet written; however review here could be useful)