Skip to content

Conversation

@ahgittin
Copy link
Contributor

Implements the behaviour discussed on ML re version syntax.

Places where OSGi syntax is required (ie registering/searching OSGi bundles) convert to the OSGi syntax. Everywhere else uses the version string exactly as supplied by the caller. Places where mappings to OSGi are not unique result in warnings.

The fixes the issue described in #672, and catalog.bom files can (should) now use familiar maven/semver versions (#.#.#-qual) which most people seem to use everywhere else. This is converted to OSGi syntax (#.#.#.qual) when creating the bundle, or comparing with installed bundles, but that's all hidden from users (unless they go deep into OSGi).

You can supply OSGi syntax (or even crazy things like v1) in the catalog.bom, and the syntax supplied is exactly what will be used in the Brooklyn catalog. There will be warnings if it does not follow the recommended syntax but they can be ignored.

This builds on (and closes) #737 and #740, depending whether people want to review those independently or in the context of this.

Merge apache/brooklyn-docs#198 once this is merged.

ahgittin added 17 commits June 16, 2017 09:49
changed a while ago to EntitySpecResolver; removes references to catalog items and hokey lookup
expand tests. some obscure ordering items are different now.
and bundle finder supports either, and does the conversion for you
support for persistence, and test
…ionedName

OSGi versions are taken at the very last moment, allowing us to preserve brooklyn-recommended syntax throughout nearly all our code; and lookups/matches in OSGi space compare the OSGi-target
@ahgittin
Copy link
Contributor Author

unrelated non-det test failure org.apache.brooklyn.entity.group.SequenceGroupTest.testGroupWithMatchingFilterReturnsEverythingThatMatches

@neykov
Copy link
Member

neykov commented Jun 26, 2017

LGTM
retest this please.

@geomacy
Copy link
Contributor

geomacy commented Jun 27, 2017

hi Alex, looks good but I did have some comments and concerns especially around the comparison of versions. I recorded the comments against #737 and #740 (and in apache/brooklyn-docs#198) rather than here, probably should just have put them here but was going in order!... let me know what you think.

@ahgittin
Copy link
Contributor Author

Updated with fixes to upstream PRs.

@geomacy
Copy link
Contributor

geomacy commented Jun 30, 2017

I've had a further think about the versions as discussed in #740. In a (long) nutshell:

  • I think we agree that there could be problems with the approach here in edge cases where people are using version formats that we don't recommend; in particular, as you note "one place this could be an issue is if we've relied on OSGi to resolve version ranges."

Re. the latter I'm imagining a case like your 'acme-cluster' (mail link) where someone has released a series of drafts, say, acme:1.0.0-v1 , .. acme:1.0.0-v9 and acme:1.0.0-v10. Things started settling down around v5, so if I have a bundle that depends on acme, and I specify an OSGI dependency on it, I would like to be able to say

Import-Package: acme;version="[1.0.0.v5, 2)"

but I will have to understand that this will exclude 1.0.0.v10 when it becomes available and write my poms/boms appropriately. That's probably something you would expect me to understand if I am writing Import-Package declarations and have read the new Brooklyn docs on versions. However it's something we should bear in mind if we introduce version ranges in Brooklyn catalogs, where if I write the following I think I should be entitled to expect it to work with v10.

classpath://acme:[1.0.0-v5,2):/catalog.bom 
  • I guess I agree that this is too far below the radar to be a deal breaker for this PR, and we can go with your suggestion of "a recommended syntax, and warn but make best effort if people not using it -- is best for now, and we can become stricter in the next version."

  • However, one final caveat, worth considering before merging: in your updates to Osgis.BundleFinder you haven't changed the comparison in findAll where the bundles are sorted in order to be able to return the latest in findOne. In the acme example above this will mean findOne will give you v9 as the latest even when v10 is out; I would argue that this should return Brooklyn's notion of what is "latest", so we should be re-mapping from OSGI bundle version to BrooklynVersion before doing the comparison. Otherwise I think it really will give us problems where the wrong bundle is picked.

Copy link
Contributor

@geomacy geomacy left a comment

Choose a reason for hiding this comment

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

Approved with one caveat in #743 (comment)

@ahgittin
Copy link
Contributor Author

very good spot on BundleFinder @geomacy - have cleaned it up so instead of sorting by version (and ignoring bundle names altogether!) it now sorts alphanumerically on name, then on version, and in the code where we findOne we take the highest version of the first entry.

as for OSGi ranges that may be a bit of a headache, or at least potentially a surprise to OSGi users. however so long as we can make it follow our scheme it should be fine: i would think anyone using a combination of qualified and unqualified versions would expect semver semantics where unqualified is last. in osgi people using qualifiers presumably always do so, and are used to using a datestamp or something similar to ensure ordering is correct and it will almost certainly still be correct. (IE I don't imagine anyone would intentionally use 1.0.0.M9 then 1.0.0.M10, or 1.0.0.M1 then 1.0.0 and want OSGi semantics to apply!)

@asfgit asfgit merged commit 7a6e06f into apache:master Jun 30, 2017
asfgit pushed a commit that referenced this pull request Jun 30, 2017
asfgit pushed a commit that referenced this pull request Jul 19, 2017
Auto wrap YAML BOMs as catalog bundles

Wraps pure-YAML BOMs uploaded to catalog in a bundle, creating a name and version if needed.

This allows us to see all items added as a unit.  This will allow us to prefer loading co-defined items (though that's not fully done, still relies on preferring search path when doing item lookup), and to edit or remove that item as a set (crucially we now keep the BOM, previously we discarded it!).

Docs changed to reflect this and explain differences in another PR.

The basic wrapping was pretty simple but a surprising number of subtleties emerged which this fixes:

* java scanning doesn't work in OSGi; docs and errors make this clear
* impacts replacement semantics (now it does a hash on the bundle to make most things the same, though a re-post of a non-SNAPSHOT BOM that doesn't declare a bundle will now fail because the random bundle name is stored on the item as containingBundle and is different on a re-post; and needed extra work to remove leaked anonymous bundles)
* had to change the way we set up the libraries / search path, to exclude this "bundle" (mainly so tests are unchanged, but also improves efficiency for simple BOMs; also the code is cleaner now)

And there is one thing that emerged that I *haven't* fixed:  on rebind it became vital to handle the situation where dependent bundles might not be loaded before the depender bundle; see failing test`testLongReferenceSequence`.  Not entirely sure how to do this /cc @neykov @geomacy @aledsage.

Depends on #743.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants