Hi, after some further thinking, I did now the following changes: - remove all usage of Java 5 features (enum, generics) and support 1.4 as target (same as the web console does) (therefore we can't use the bnd annotations) - removed the manager and handler interfaces - right now these are not needed and we can add them once we really need them - without the manager, category doesn't make sense, therefore I removed this as well - ZipAttachementProvider does not inherit from InventoryPrinter anymore - The printers are registered with the category "Status" for the web console
Regards Carsten 2013/2/24 Carsten Ziegeler <[email protected]>: > 2013/2/23 Felix Meschberger <[email protected]>: >> Hi >> >> I am looking into cutting a Web Console release soon, which will require a >> release of the "inventory" bundle first. This is the bundle previously >> called status having been renamed because the maven module name >> "status-printer" was not congruent with the bundle symbolic name >> o.a.f.status. >> >> I have been looking into this module a bit and would like to share some >> feedback: > Great! > >> >> * IIUIC an InventoryPrinter registered with a .category property will be >> registered with the web console as a proxy plugin with the respective >> property (allowing the InventoryPrinter to be accessed by its own URL). If >> the .category property is not set, the default Inventory category is used. I >> think we should not have such a .category property and these adapter plugins >> should always be registered with the Inventory category. > > Actually the category property is used for two purposes - the > Inventory stuff can be used without the web console, that's one of the > main reasons to create this separate bundle in the first place. So I > think it makes sense to have a categorization of printers when using > the inventory API directly (see below). > >> >> * Configurability: Should the inventory- URL prefix and the Inventory >> category be configurable ? > I think the category, yes - as this is usable outside of the web > console - prefix, no > >> >> * Should the ZipAttachementProvider really extend the InventoryPrinter >> interface ? I rather think it should not extend the InventoryPrinter >> interface. Rather it should be used as a "marker with API": When writing the >> ZIP, the InventoryPrinter services are checked whether they also implement >> the ZipAttachementProvider and should be called accordingly. > The ZipAttachmentProvider is in fact an extension of the > InventoryPrinter - so why hide this fact? The implementation always > checks all inventory printers whether they implement this interface, > so I see no benefit of having this a separate marker interface. > >> >> * Minor: Source Code Formatting should be adapted to our coding conventions. >> >> * What are the InventoryPrinterHandler and InventoryPrinterManager >> interfaces used for ? Looks like they are used internally in the Inventory >> bundle -- are they used externally ? I have the impression, this API is not >> required. > They can be used by any client code to get inventory printers - so for > example if you want to write a gogo shell extension printing out all > information from the inventory printers, these extension can use the > printer manager to get these things. I've added the handler interface > as this one makes handling the printers easiers by client code and > clearly states that client code should never use the printer services > directly. > Of course one could argue that client code could lookup all printers > and use them directly, however this would duplicate all the logic we > already have implemented and therefore I came up with the > manager/handler API for this. > >> >> * API: We should probably tag the interfaces with the BND @ConsumerType and >> @ProviderType annotations. > Yepp > >> >> * While I think "Inventory" as a name is probably better (and less >> confusing) than Configuration Status or Status or Status Printer, I am not >> really convinced (and still fear the name is confusing). Maybe some native >> english speaker has a better name for that ? > Yepp good idea - I never liked the status/status printer name and > after some searching came across "inventory" as a name as this is > nearly used anywhere, so we avoid naming confusing with existing > stuff. But then, I'm not a native speaker either. > > Carsten >> >> Regards >> Felix >> >> PS: Releasing the Inventory bundle blocks the Web Console release because >> the "all-in-one" profile of the Web Console embeds the Inventory bundle. > > > > -- > Carsten Ziegeler > [email protected] -- Carsten Ziegeler [email protected]
