brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From geomacy <...@git.apache.org>
Subject [GitHub] brooklyn-server pull request #608: `BundleMaker` REST call allowing to add Z...
Date Fri, 24 Mar 2017 17:45:52 GMT
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message