Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SPI Config API for Jersey #4116

Merged
merged 3 commits into from
Jun 4, 2019
Merged

SPI Config API for Jersey #4116

merged 3 commits into from
Jun 4, 2019

Conversation

senivam
Copy link
Contributor

@senivam senivam commented Apr 25, 2019

Relates to #4110

Signed-off-by: Maxim Nesen maxim.nesen@oracle.com

@arjantijms
Copy link
Contributor

Just wondering, but why are Helidon and SmallRye providers needed? Would it not be an option to only use the public MicroProfile Config APIs?

@senivam
Copy link
Contributor Author

senivam commented Apr 25, 2019

Actually those are external modules, they are not needed, just optional. The aim is to demonstrate possibilities to plug different implementations of MP configurations.

And in the same way Jersey provides more tooling prepared out of the box so particular user is not required to implement s/his own connector if existing fits requirements.

@smillidge
Copy link

smillidge commented Apr 26, 2019 •
edited
Loading

But can't you just use the vanilla MicroProfile API to get the Config without these providers?

ConfigProvider.getConfig();

then whatever Config Provider is available will be used. There shouldn't be any need to use apis from the provider directly. Config Provider uses a Service Loader to find the provider on the classpath without coupling to non-standard apis.

@jansupol
Copy link
Contributor

jansupol commented Apr 26, 2019 •
edited
Loading

@smillidge Using the MP config is just one possibility. All this should be able to

  1. work with an internal environment variables provider
  2. any Jersey specific config facade provider providing
  • MP Config
  • Config JSR config (should it ever be finished)
  • any other user provided config provider, which could be able to provide properties from other sources (XML, DB, any other remote property service,...)

That is to explain why there is non-mp-config SPI used

@senivam
Copy link
Contributor Author

senivam commented Apr 26, 2019

OK, I will introduce some minor changes in a while (according to your comments)

@smillidge
Copy link

@jansupol I like the whole config provider SPI. I was commenting more in the context of creating the MP Config provider as an implementation of the SPI there shouldn't be the need to create a dependency to the specific MP Config implementations as the MP ConfigProvider itself should provide that abstraction.

@senivam senivam changed the title MP-Config SPI API for Jersey SPI Config API for Jersey Apr 29, 2019
@senivam
Copy link
Contributor Author

senivam commented Apr 29, 2019

@smillidge, @arjantijms, @jansupol I've updated the PR, build is passed OK. Could you please take a look?

@senivam senivam requested a review from jansupol April 30, 2019 05:58
@senivam senivam force-pushed the config branch 2 times, most recently from 51fd0c4 to 54e2220 Compare May 2, 2019 06:46
jansupol
jansupol reviewed May 10, 2019
View reviewed changes
pom.xml Outdated
@@ -2043,6 +2043,9 @@
<weld3.version>3.0.0.Final</weld3.version>
<xerces.version>2.11.0</xerces.version>
<yasson.version>1.0.3</yasson.version>
<helidon.version>1.0.2</helidon.version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CQ required for Helidon, MP Config API, smallrye

Copy link
Contributor Author

@senivam senivam May 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CQ#: 19855
CQ#: 19756

jansupol
jansupol reviewed May 10, 2019
View reviewed changes
pom.xml Outdated Show resolved Hide resolved
@jansupol
Copy link
Contributor

jansupol commented May 18, 2019 •
edited
Loading

Given the issue with smallrye licensing, we would probably need to split the commit into two, the test module with smallrye in a separate commit. I assume that one will not make it to 2.29

@senivam senivam force-pushed the config branch 3 times, most recently from 04e944d to 4a871a9 Compare May 21, 2019 07:07
@senivam
Copy link
Contributor Author

senivam commented May 21, 2019

Short summary on current state of the PR:

Config engine is modified to be plugged automatically for both client and server parts of Jersey.

Tests are adjusted accordingly.

Small-ray test/example is removed from the PR due to license header error in one of its sources. Will be added separately.

@senivam
Config SPI API for Jersey
4148473 
Signed-off-by: Maxim Nesen <maxim.nesen@oracle.com>
jansupol
jansupol reviewed May 28, 2019
View reviewed changes
ext/microprofile/config/pom.xml Outdated
</parent>
<modelVersion>4.0.0</modelVersion>

<artifactId>config</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please align the name with other ext projects: jersey-mp-config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't that you who told me to avoid MP naming in modules/PRs?

jansupol
jansupol reviewed May 28, 2019
View reviewed changes
ext/pom.xml
@@ -44,6 +44,7 @@
<module>cdi</module>
<module>entity-filtering</module>
<module>metainf-services</module>
<module>microprofile/config</module>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just microprofile. Each sub module should be dealt with in the microprofile project. There would be at least 2 submodules

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, but we already have a PR #4086 which does this with its module, so it would be less conflicts when both PR's will be merged

jansupol
jansupol reviewed May 28, 2019
View reviewed changes
...n/src/main/java/org/glassfish/jersey/internal/config/ExternalPropertiesAutoDiscoverable.java Outdated
public void configure(FeatureContext context) {
if (!context.getConfiguration().isRegistered(ExternalPropertiesConfigurationFactoryFeature.class)) {
ExternalPropertiesConfigurationFactoryFeature.getFactory().configure(context);
context.register(ExternalPropertiesConfigurationFactoryFeature.getFactory());
Copy link
Contributor

@jansupol jansupol May 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would register the feature twice. Context.register(ExternalPropertiesConfigurationFactoryFeature.class)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) I saw that. It's due to mixing the feature and the factory

@senivam senivam force-pushed the config branch 4 times, most recently from d5cbc91 to 9371a4f Compare May 30, 2019 10:19
@senivam
resolving comments from 27.5.2019
1355ee4 
Signed-off-by: Maxim Nesen <maxim.nesen@oracle.com>
@senivam
resolving comments from 30.5.2019
633f289 
Signed-off-by: Maxim Nesen <maxim.nesen@oracle.com>
m0mus
m0mus approved these changes Jun 3, 2019
View reviewed changes
Copy link
Member

@m0mus m0mus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@senivam senivam merged commit 5dec448 into eclipse-ee4j:master Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jansupol jansupol jansupol approved these changes

@m0mus m0mus m0mus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@senivam @arjantijms @smillidge @jansupol @m0mus