Improve Brooklyn-management of OSGi-already-managed bundles#842
Conversation
take the URL from the existing osgi-installed bundle. much better logging.
needed for tests now that we do more with bundles supplied in brooklyn.libraries
makes behaviour consistent with dist behaviour regarding karaf urls
geomacy
left a comment
geomacy
left a comment
There was a problem hiding this comment.
Broadly this looks good to me, a couple of trivial comments; but it is pretty complex code and I'm sure there could be subtleties lurking in there that I have missed.
| } else { | ||
| // if bundle is not brooklyn-managed we want to make it be so | ||
| // and for that we need to find a URL. | ||
| // some things declare usable locations, though these might be maven and |
There was a problem hiding this comment.
text ends just with and - did you mean to put something more?
| result.metadata = new BasicManagedBundle(installedBundle.get().getSymbolicName(), installedBundle.get().getVersion().toString(), | ||
| suppliedKnownBundleMetadata!=null ? suppliedKnownBundleMetadata.getUrl() : null); | ||
|
|
||
| assert zipIn!=null || suppliedKnownBundleMetadata.getUrl()!=null : "should have found a stream or inferred a URL"; |
There was a problem hiding this comment.
Do you really mean to use normal Java assert here? If it's worth asserting here is it not worth asserting normally at run time ie. without having to configure JVM -ea?
There was a problem hiding this comment.
the assertion was more for a person reading it
| // get a handle on the zip file (although we could skip if not doing persistence - but that feels even worse than this!) | ||
| try { | ||
| url = Strings.removeFromStart(url, "system:"); | ||
| File zipTemp = new BundleMaker(ResourceUtils.create()).createJarFromClasspathDir(url); |
There was a problem hiding this comment.
I take it that createJarFromClasspathDir works ok in this case, where the url will start with file:.
There was a problem hiding this comment.
yep, ClassLoaderUtilsTest depends on this and it passes. pretty cool.
|
all addressed -- thx @geomacy, merging |
3 participants
Previously could claim it was already installed but not brooklyn-manage it.
Now it is much smarter about checking whether what is being supplied now (by reference or by contents) is an exact match and if so, don't uninstall and reinstall it.
Some annoying issues fixing tests due to how karaf/java deals with
target/classes/bundles in dev, but tests are all working too now.NB this requires #810 due to conflicts in osgi bundle removal semantics