lightblue-platform / lightblue-applications

GNU General Public License v3.0
5 stars 9 forks source link

Fixes #99: Using LightblueMetadataProxyServlet instead of deprecated LightblueProxyServlet #100

Closed paterczm closed 9 years ago

paterczm commented 9 years ago

https://github.com/lightblue-platform/lightblue-applications/issues/99

Related puppet changes: https://github.com/lightblue-platform/lightblue-puppet/pull/133

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-4.83%) to 17.39% when pulling 9ebf7a9617f2e6fbc102cf2f982a6a674bef6a1a on paterczm:master into 04f9e565b0cf0adab0c64a1d203065027902570e on lightblue-platform:master.

jewzaam commented 9 years ago

@paterczm Could you explain why ApplicationContext was added? I don't see it used.

paterczm commented 9 years ago

@jewzaam ApplicationContext is a CDI producer used to produce dependencies for the metadata servlet. See https://github.com/lightblue-platform/lightblue-client/blob/master/http/src/main/java/com/redhat/lightblue/client/http/servlet/LightblueMetadataProxyServlet.java.

jewzaam commented 9 years ago

@paterczm I don't see any reference to the application context. In this PR I see a new class created but no use of that class. Given the link you provided, would this be used by the client in a future PR?

paterczm commented 9 years ago

The CDI container is using ApplicationContext class. Annotations tie it all together. If I take this class away, the LightblueMetadataProxyServlet's constructor will not receive the arguments it needs.

alechenninger commented 9 years ago

@paterczm It certainly doesn't hurt (and you could argue it's better to be explicit), but producing a lightblue client configuration is optional. By default it will do what you put in ApplicationContext. See:

https://github.com/lightblue-platform/lightblue-client/blob/master/http/src/main/java/com/redhat/lightblue/client/http/servlet/LightblueServletContextConfiguration.java#L166

The only difference is that by default, servlet init parameters will be looked at which can override configuration. But it falls back to PropertiesLightblueClientConfiguration.fromDefault() for any that are not overridden in the web.xml.

alechenninger commented 9 years ago

Also see: https://github.com/lightblue-platform/lightblue-client/blob/master/http/src/test/java/com/redhat/lightblue/client/http/servlet/AbstractLightblueProxyServletTest.java#L123

alechenninger commented 9 years ago

Nevermind, you're using it for the http client which makes sense... sorry, nothing to see here :)

paterczm commented 9 years ago

@alechenninger, I'm not sure what you're proposing here - did you cancel only your 2nd post, or both?

alechenninger commented 9 years ago

@paterczm Sorry for confusion, ignore both. I was proposing removing the getLightblueClientConfiguration method but you're using that to make the http client, so it makes sense to keep it.

dcrissman commented 9 years ago

@paterczm if ApplicationContext is consumed by lightblue-client, then does that class belong in that library instead lightblue-application?

paterczm commented 9 years ago

ApplicationContext is application specific configuration of the LightblueMetadataProxyServlet. Perhaps this is more flexibility than we really need, but his is how it was designed.