Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/608#discussion_r107961617
--- Diff:
rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java
---
@@ -245,13 +246,21 @@ public Response createFromArchive(byte[] zipInput) {
if
(!BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_LOAD_BUNDLE_CATALOG_BOM))
{
// if the above feature is not enabled, let's do it
manually (as a contract of this method)
- new CatalogBomScanner().new CatalogPopulator(
- ((LocalManagementContext)
mgmt()).getOsgiManager().get().getFramework().getBundleContext(),
- mgmt()).addingBundle(bundle, null);
+ try {
+ new CatalogBundleLoader(new CatalogBomScanner(),
mgmt()).scanForCatalog(bundle);
--- End diff --
This is a bit of a problem; you shouldn't have to pass a
`CatalogBomScanner()` in here; the point of doing this decoupling was so that
the `CatalogBundleLoader` could avoid knowing about the `CatalogBomScanner`.
I see that the use that is made of this is handling the
[whitelist/blacklist](https://github.com/apache/brooklyn-server/commit/7046ba59cefd7e82ffad3f97057b3646915d1f08#diff-c46e6ef1f0a91ac1f58e7a340ecc0f34)
functionality. (Well there is also `CatalogBomScanner.bundleIds()` but that
can be made a static method,
and maybe moved to `CatalogUtils`.)
This whitelist stuff is configured
[here](https://github.com/apache/brooklyn-server/blob/master/karaf/init/src/main/resources/OSGI-INF/blueprint/blueprint.xml#L123),
in the definition of the singleton scanner object. (Only supported in Karaf
launcher at the moment, as you see.)
Doing `new CatalogBundleLoader(new CatalogBomScanner(),
mgmt()).scanForCatalog(bundle);` means you are supplying an object that hasn't
been configured with the right white/blacklists, so it bypasses this
restriction.
I'm going to suggest this is something we can live with for now, but mark
as a "TODO" for future action.
However, it is still ugly to have the dependency on the `CatalogBomScanner`
in the tracker and the loader, so I suggest we replace it with a simple
predicate that wraps the whitelist checks and inject the predicate into the
loader. That way the CatalogBomScanner can inject the correct whitelist
configuration in the blueprint.xml, while in the REST API here we just live
with an "always true" for the moment.
The loader could also be created in the scanner and injected into the
tracker, which will simplify the latter a bit.
That's all a bit vague; I have to play around with the code a bit to help
me think, so here's a PR that
illustrates what I mean - https://github.com/tbouron/brooklyn-server/pull/2
What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
