Increased visibility of EnvironmentPropertyConfigSourceProvider?

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Increased visibility of EnvironmentPropertyConfigSourceProvider?

John D. Ament
Hi,

I have a use case for leveraging something
like EnvironmentPropertyConfigSourceProvider to load multiple property
files, outside of apache-deltaspike.properties.  Right now its package
local and in the impl JAR.  I'd rather not duplicate the logic since whats
being done is exactly what is needed.  I was wondering if anyone had any
concerns with making this an API class instead of an impl class?

John
Reply | Threaded
Open this post in threaded view
|

Re: Increased visibility of EnvironmentPropertyConfigSourceProvider?

Romain Manni-Bucau
Hi John,

think it is true for most of your ConfigSource[Provider] base classes in
config-impl. I got the need of MapConfigSource for instance and needed to
"duplicate" it several time. Think it is time to move several helpers we
use internally to the API since users can need them as well. Surely few
cosmetic to change (adding some constructor probably) but can worth it.

(in other terms: +1)


Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> |  Blog
<https://blog-rmannibucau.rhcloud.com> | Old Wordpress Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Tomitriber
<http://www.tomitribe.com> | JavaEE Factory
<https://javaeefactory-rmannibucau.rhcloud.com>

2016-08-21 15:28 GMT+02:00 John D. Ament <[hidden email]>:

> Hi,
>
> I have a use case for leveraging something
> like EnvironmentPropertyConfigSourceProvider to load multiple property
> files, outside of apache-deltaspike.properties.  Right now its package
> local and in the impl JAR.  I'd rather not duplicate the logic since whats
> being done is exactly what is needed.  I was wondering if anyone had any
> concerns with making this an API class instead of an impl class?
>
> John
>
Reply | Threaded
Open this post in threaded view
|

Re: Increased visibility of EnvironmentPropertyConfigSourceProvider?

John D. Ament
Hi Romain,

You're correct, there's a lot of useful abstract classes in impl that could
be API.  Makes it easier to build extensions.

But on my original question, I forgot all about PropertyFileConfig which is
for my use case.

I also just stumbled on to the fact that
https://deltaspike.apache.org/documentation/configuration.html isn't linked
from the documentation landing page (which explains a few other pain
points).  I'll fix shortly.

John

On Sun, Aug 21, 2016 at 9:33 AM Romain Manni-Bucau <[hidden email]>
wrote:

> Hi John,
>
> think it is true for most of your ConfigSource[Provider] base classes in
> config-impl. I got the need of MapConfigSource for instance and needed to
> "duplicate" it several time. Think it is time to move several helpers we
> use internally to the API since users can need them as well. Surely few
> cosmetic to change (adding some constructor probably) but can worth it.
>
> (in other terms: +1)
>
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> <https://blog-rmannibucau.rhcloud.com> | Old Wordpress Blog
> <http://rmannibucau.wordpress.com> | Github <
> https://github.com/rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Tomitriber
> <http://www.tomitribe.com> | JavaEE Factory
> <https://javaeefactory-rmannibucau.rhcloud.com>
>
> 2016-08-21 15:28 GMT+02:00 John D. Ament <[hidden email]>:
>
> > Hi,
> >
> > I have a use case for leveraging something
> > like EnvironmentPropertyConfigSourceProvider to load multiple property
> > files, outside of apache-deltaspike.properties.  Right now its package
> > local and in the impl JAR.  I'd rather not duplicate the logic since
> whats
> > being done is exactly what is needed.  I was wondering if anyone had any
> > concerns with making this an API class instead of an impl class?
> >
> > John
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: Increased visibility of EnvironmentPropertyConfigSourceProvider?

Mark Struberg-3
In reply to this post by John D. Ament
EnvironmentPropertyConfigSourceProvider is effectively an internal class. And thus I'd not move it to the API. Just look how much we would need to add to the api package.

I understand that you don't like to duplicate code.

Usually things like ConfigSources are totally orthogonal to your production code. So you need those only as runtime dependency.


In that case just make a tool module which has ds-core-impl as compile time dependency and then you can simply re-use and even extend the internal class.
But that class is clearly NOT an API class imo.

LieGrue,
strub





> On Sunday, 21 August 2016, 15:28, John D. Ament <[hidden email]> wrote:
> > Hi,
>
> I have a use case for leveraging something
> like EnvironmentPropertyConfigSourceProvider to load multiple property
> files, outside of apache-deltaspike.properties.  Right now its package
> local and in the impl JAR.  I'd rather not duplicate the logic since whats
> being done is exactly what is needed.  I was wondering if anyone had any
> concerns with making this an API class instead of an impl class?
>
> John
>
Reply | Threaded
Open this post in threaded view
|

Re: Increased visibility of EnvironmentPropertyConfigSourceProvider?

Romain Manni-Bucau
2016-08-21 16:08 GMT+02:00 Mark Struberg <[hidden email]>:

> EnvironmentPropertyConfigSourceProvider is effectively an internal class.
> And thus I'd not move it to the API. Just look how much we would need to
> add to the api package.
>
> I understand that you don't like to duplicate code.
>
> Usually things like ConfigSources are totally orthogonal to your
> production code. So you need those only as runtime dependency.
>
>
I get the idea but this is likely not what you wanted to say. Typically
sources are production code (but not business code).


>
> In that case just make a tool module which has ds-core-impl as compile
> time dependency and then you can simply re-use and even extend the internal
> class.
> But that class is clearly NOT an API class imo.
>
>
Question is: how stable do we consider -impl is? If the solution to that
issue is 1. make the class (whichever it is) public 2. say to the user to
import -impl in his pom, then we need to show that the API is stable and
not just an internal we can break when we want. @Stable would be an option
but moving it to -api is another one. Don't care of the solution while we
prevent the user to duplicate our -impl code.


> LieGrue,
> strub
>
>
>
>
>
> > On Sunday, 21 August 2016, 15:28, John D. Ament <[hidden email]>
> wrote:
> > > Hi,
> >
> > I have a use case for leveraging something
> > like EnvironmentPropertyConfigSourceProvider to load multiple property
> > files, outside of apache-deltaspike.properties.  Right now its package
> > local and in the impl JAR.  I'd rather not duplicate the logic since
> whats
> > being done is exactly what is needed.  I was wondering if anyone had any
> > concerns with making this an API class instead of an impl class?
> >
> > John
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: Increased visibility of EnvironmentPropertyConfigSourceProvider?

Gerhard Petracek
Administrator
In reply to this post by Mark Struberg-3
@mark: +1

regards,
gerhard



2016-08-21 16:08 GMT+02:00 Mark Struberg <[hidden email]>:

> EnvironmentPropertyConfigSourceProvider is effectively an internal class.
> And thus I'd not move it to the API. Just look how much we would need to
> add to the api package.
>
> I understand that you don't like to duplicate code.
>
> Usually things like ConfigSources are totally orthogonal to your
> production code. So you need those only as runtime dependency.
>
>
> In that case just make a tool module which has ds-core-impl as compile
> time dependency and then you can simply re-use and even extend the internal
> class.
> But that class is clearly NOT an API class imo.
>
> LieGrue,
> strub
>
>
>
>
>
> > On Sunday, 21 August 2016, 15:28, John D. Ament <[hidden email]>
> wrote:
> > > Hi,
> >
> > I have a use case for leveraging something
> > like EnvironmentPropertyConfigSourceProvider to load multiple property
> > files, outside of apache-deltaspike.properties.  Right now its package
> > local and in the impl JAR.  I'd rather not duplicate the logic since
> whats
> > being done is exactly what is needed.  I was wondering if anyone had any
> > concerns with making this an API class instead of an impl class?
> >
> > John
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: Increased visibility of EnvironmentPropertyConfigSourceProvider?

Romain Manni-Bucau
Mark/Gerhard: are you against the feature or packaging?

Le 21 août 2016 16:37, "Gerhard Petracek" <[hidden email]> a
écrit :

> @mark: +1
>
> regards,
> gerhard
>
>
>
> 2016-08-21 16:08 GMT+02:00 Mark Struberg <[hidden email]>:
>
> > EnvironmentPropertyConfigSourceProvider is effectively an internal
> class.
> > And thus I'd not move it to the API. Just look how much we would need to
> > add to the api package.
> >
> > I understand that you don't like to duplicate code.
> >
> > Usually things like ConfigSources are totally orthogonal to your
> > production code. So you need those only as runtime dependency.
> >
> >
> > In that case just make a tool module which has ds-core-impl as compile
> > time dependency and then you can simply re-use and even extend the
> internal
> > class.
> > But that class is clearly NOT an API class imo.
> >
> > LieGrue,
> > strub
> >
> >
> >
> >
> >
> > > On Sunday, 21 August 2016, 15:28, John D. Ament <[hidden email]
> >
> > wrote:
> > > > Hi,
> > >
> > > I have a use case for leveraging something
> > > like EnvironmentPropertyConfigSourceProvider to load multiple property
> > > files, outside of apache-deltaspike.properties.  Right now its package
> > > local and in the impl JAR.  I'd rather not duplicate the logic since
> > whats
> > > being done is exactly what is needed.  I was wondering if anyone had
> any
> > > concerns with making this an API class instead of an impl class?
> > >
> > > John
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: Increased visibility of EnvironmentPropertyConfigSourceProvider?

Mark Struberg-3
In reply to this post by Romain Manni-Bucau
Oh if it's not public then we should really make it!

You are right with 'production code'. Of course it's in production.
But it's more like infrastructure glue code and not business related, right?
Means your colleagues tinkering on UI and business should typically not even get it as compile scoped dependency but really only as runtime dep.
John, romain, do you agree on this point?
Just trying to make a few steps back to look at the problem from a distance...

LieGrue,
Strub

> Am 21.08.2016 um 16:11 schrieb Romain Manni-Bucau <[hidden email]>:
>
> 2016-08-21 16:08 GMT+02:00 Mark Struberg <[hidden email]>:
>
>> EnvironmentPropertyConfigSourceProvider is effectively an internal class.
>> And thus I'd not move it to the API. Just look how much we would need to
>> add to the api package.
>>
>> I understand that you don't like to duplicate code.
>>
>> Usually things like ConfigSources are totally orthogonal to your
>> production code. So you need those only as runtime dependency.
> I get the idea but this is likely not what you wanted to say. Typically
> sources are production code (but not business code).
>
>
>>
>> In that case just make a tool module which has ds-core-impl as compile
>> time dependency and then you can simply re-use and even extend the internal
>> class.
>> But that class is clearly NOT an API class imo.
> Question is: how stable do we consider -impl is? If the solution to that
> issue is 1. make the class (whichever it is) public 2. say to the user to
> import -impl in his pom, then we need to show that the API is stable and
> not just an internal we can break when we want. @Stable would be an option
> but moving it to -api is another one. Don't care of the solution while we
> prevent the user to duplicate our -impl code.
>
>
>> LieGrue,
>> strub
>>
>>
>>
>>
>>
>>> On Sunday, 21 August 2016, 15:28, John D. Ament <[hidden email]>
>> wrote:
>>>> Hi,
>>>
>>> I have a use case for leveraging something
>>> like EnvironmentPropertyConfigSourceProvider to load multiple property
>>> files, outside of apache-deltaspike.properties.  Right now its package
>>> local and in the impl JAR.  I'd rather not duplicate the logic since
>> whats
>>> being done is exactly what is needed.  I was wondering if anyone had any
>>> concerns with making this an API class instead of an impl class?
>>>
>>> John
>>

Reply | Threaded
Open this post in threaded view
|

Re: Increased visibility of EnvironmentPropertyConfigSourceProvider?

Romain Manni-Bucau
Le 21 août 2016 20:44, "Mark Struberg" <[hidden email]> a écrit :
>
> Oh if it's not public then we should really make it!
>
> You are right with 'production code'. Of course it's in production.
> But it's more like infrastructure glue code and not business related,
right?
> Means your colleagues tinkering on UI and business should typically not
even get it as compile scoped dependency but really only as runtime dep.
> John, romain, do you agree on this point?

So not what we are talking about and my point ;). If we care then you dev
against config source api.

Sounds like we can need a config-support module for such a case if you case
of visibility

> Just trying to make a few steps back to look at the problem from a
distance...

>
> LieGrue,
> Strub
>
> > Am 21.08.2016 um 16:11 schrieb Romain Manni-Bucau <[hidden email]
>:
> >
> > 2016-08-21 16:08 GMT+02:00 Mark Struberg <[hidden email]>:
> >
> >> EnvironmentPropertyConfigSourceProvider is effectively an internal
class.
> >> And thus I'd not move it to the API. Just look how much we would need
to

> >> add to the api package.
> >>
> >> I understand that you don't like to duplicate code.
> >>
> >> Usually things like ConfigSources are totally orthogonal to your
> >> production code. So you need those only as runtime dependency.
> > I get the idea but this is likely not what you wanted to say. Typically
> > sources are production code (but not business code).
> >
> >
> >>
> >> In that case just make a tool module which has ds-core-impl as compile
> >> time dependency and then you can simply re-use and even extend the
internal
> >> class.
> >> But that class is clearly NOT an API class imo.
> > Question is: how stable do we consider -impl is? If the solution to that
> > issue is 1. make the class (whichever it is) public 2. say to the user
to
> > import -impl in his pom, then we need to show that the API is stable and
> > not just an internal we can break when we want. @Stable would be an
option
> > but moving it to -api is another one. Don't care of the solution while
we

> > prevent the user to duplicate our -impl code.
> >
> >
> >> LieGrue,
> >> strub
> >>
> >>
> >>
> >>
> >>
> >>> On Sunday, 21 August 2016, 15:28, John D. Ament <[hidden email]
>
> >> wrote:
> >>>> Hi,
> >>>
> >>> I have a use case for leveraging something
> >>> like EnvironmentPropertyConfigSourceProvider to load multiple property
> >>> files, outside of apache-deltaspike.properties.  Right now its package
> >>> local and in the impl JAR.  I'd rather not duplicate the logic since
> >> whats
> >>> being done is exactly what is needed.  I was wondering if anyone had
any
> >>> concerns with making this an API class instead of an impl class?
> >>>
> >>> John
> >>
>
Reply | Threaded
Open this post in threaded view
|

Re: Increased visibility of EnvironmentPropertyConfigSourceProvider?

Mark Struberg-3
In reply to this post by Romain Manni-Bucau
I like that feature. But I don't like to pollute our pretty clean API with it ;)

At the end it's an impl detail whether you duplicate it, write it your own, or reuse the class from ds-core-impl. (Assuming we make necessary parts public).
Ofc we give clearly less guarantees for stability than our API classes. But it should remain considerably stable in the foreseeable future I think.

LieGrue,
Strub

> Am 21.08.2016 um 17:12 schrieb Romain Manni-Bucau <[hidden email]>:
>
> Mark/Gerhard: are you against the feature or packaging?
>
> Le 21 août 2016 16:37, "Gerhard Petracek" <[hidden email]> a
> écrit :
>
>> @mark: +1
>>
>> regards,
>> gerhard
>>
>>
>>
>> 2016-08-21 16:08 GMT+02:00 Mark Struberg <[hidden email]>:
>>
>>> EnvironmentPropertyConfigSourceProvider is effectively an internal
>> class.
>>> And thus I'd not move it to the API. Just look how much we would need to
>>> add to the api package.
>>>
>>> I understand that you don't like to duplicate code.
>>>
>>> Usually things like ConfigSources are totally orthogonal to your
>>> production code. So you need those only as runtime dependency.
>>>
>>>
>>> In that case just make a tool module which has ds-core-impl as compile
>>> time dependency and then you can simply re-use and even extend the
>> internal
>>> class.
>>> But that class is clearly NOT an API class imo.
>>>
>>> LieGrue,
>>> strub
>>>
>>>
>>>
>>>
>>>
>>>> On Sunday, 21 August 2016, 15:28, John D. Ament <[hidden email]
>>>
>>> wrote:
>>>>> Hi,
>>>>
>>>> I have a use case for leveraging something
>>>> like EnvironmentPropertyConfigSourceProvider to load multiple property
>>>> files, outside of apache-deltaspike.properties.  Right now its package
>>>> local and in the impl JAR.  I'd rather not duplicate the logic since
>>> whats
>>>> being done is exactly what is needed.  I was wondering if anyone had
>> any
>>>> concerns with making this an API class instead of an impl class?
>>>>
>>>> John
>>

Reply | Threaded
Open this post in threaded view
|

Re: Increased visibility of EnvironmentPropertyConfigSourceProvider?

John D. Ament
In reply to this post by Mark Struberg-3
On Sun, Aug 21, 2016 at 2:44 PM Mark Struberg <[hidden email]>
wrote:

> Oh if it's not public then we should really make it!
>
> You are right with 'production code'. Of course it's in production.
> But it's more like infrastructure glue code and not business related,
> right?
> Means your colleagues tinkering on UI and business should typically not
> even get it as compile scoped dependency but really only as runtime dep.
> John, romain, do you agree on this point?
>

Right, the problem is that the class isn't even public.  So even if I
declare a proper dependency on it, can't be used.  I would say my view
point is that its SPI, along with a few other classes (MapConfigSource,
PropertiesConfigSource, PropertyFileConfigSource).  Realistically though,
the problem I was trying to solve was resolved by implementing
"PropertyFileConfig" but trying to remember how that should work, I
couldn't even find the doc page (eventually found it, its mentioned briefly
on core, but really should be its own link on the landing page, which I
changed).

I could also see the class not being SPI.  As long as things are configured
properly, its not needed.  That's assuming we have a dynamic way to
register more property files.  Right now, that's not possible
(PropertyFileConfig is only read once, and only via ServiceLoader).

Maybe thats the better discussion to have.


> Just trying to make a few steps back to look at the problem from a
> distance...
>
> LieGrue,
> Strub
>
> > Am 21.08.2016 um 16:11 schrieb Romain Manni-Bucau <[hidden email]
> >:
> >
> > 2016-08-21 16:08 GMT+02:00 Mark Struberg <[hidden email]>:
> >
> >> EnvironmentPropertyConfigSourceProvider is effectively an internal
> class.
> >> And thus I'd not move it to the API. Just look how much we would need to
> >> add to the api package.
> >>
> >> I understand that you don't like to duplicate code.
> >>
> >> Usually things like ConfigSources are totally orthogonal to your
> >> production code. So you need those only as runtime dependency.
> > I get the idea but this is likely not what you wanted to say. Typically
> > sources are production code (but not business code).
> >
> >
> >>
> >> In that case just make a tool module which has ds-core-impl as compile
> >> time dependency and then you can simply re-use and even extend the
> internal
> >> class.
> >> But that class is clearly NOT an API class imo.
> > Question is: how stable do we consider -impl is? If the solution to that
> > issue is 1. make the class (whichever it is) public 2. say to the user to
> > import -impl in his pom, then we need to show that the API is stable and
> > not just an internal we can break when we want. @Stable would be an
> option
> > but moving it to -api is another one. Don't care of the solution while we
> > prevent the user to duplicate our -impl code.
> >
> >
> >> LieGrue,
> >> strub
> >>
> >>
> >>
> >>
> >>
> >>> On Sunday, 21 August 2016, 15:28, John D. Ament <[hidden email]
> >
> >> wrote:
> >>>> Hi,
> >>>
> >>> I have a use case for leveraging something
> >>> like EnvironmentPropertyConfigSourceProvider to load multiple property
> >>> files, outside of apache-deltaspike.properties.  Right now its package
> >>> local and in the impl JAR.  I'd rather not duplicate the logic since
> >> whats
> >>> being done is exactly what is needed.  I was wondering if anyone had
> any
> >>> concerns with making this an API class instead of an impl class?
> >>>
> >>> John
> >>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Increased visibility of EnvironmentPropertyConfigSourceProvider?

Mark Struberg-3
Thanks for adding a link to the config stuff in a more visible place!

Yes, I also think we should have those classes public (but in the impl module).

Regarding the PropertyFileConfig:
I think I documented this, but better mention it again:
You can register it in 2 ways

1.) Simply implement the interface
 -> in that case it gets picked up via ProcessAnnotatedType and will _only_ be available after the container did finish booting!
 That means that you cannot use values from those ConfigSources during CDI boot, e.g. for @Exclude

2.) To work around the timing issue in 1 we later also made it pick up via java.util.ServiceLoader. In that case it will get picked up like a regular ConfigSource and is also available during container bootstrap.

Just don't fortet to add a @Exclude annotation to prevent the PropertyFileConfig from being picked up twice.


Happy to get any feedback about how to improve the docs if something is missing.


LieGrue,
strub




> On Sunday, 21 August 2016, 20:57, John D. Ament <[hidden email]> wrote:
> > On Sun, Aug 21, 2016 at 2:44 PM Mark Struberg <[hidden email]>
> wrote:
>
>>  Oh if it's not public then we should really make it!
>>
>>  You are right with 'production code'. Of course it's in
> production.
>>  But it's more like infrastructure glue code and not business related,
>>  right?
>>  Means your colleagues tinkering on UI and business should typically not
>>  even get it as compile scoped dependency but really only as runtime dep.
>>  John, romain, do you agree on this point?
>>
>
> Right, the problem is that the class isn't even public.  So even if I
> declare a proper dependency on it, can't be used.  I would say my view
> point is that its SPI, along with a few other classes (MapConfigSource,
> PropertiesConfigSource, PropertyFileConfigSource).  Realistically though,
> the problem I was trying to solve was resolved by implementing
> "PropertyFileConfig" but trying to remember how that should work, I
> couldn't even find the doc page (eventually found it, its mentioned briefly
> on core, but really should be its own link on the landing page, which I
> changed).
>
> I could also see the class not being SPI.  As long as things are configured
> properly, its not needed.  That's assuming we have a dynamic way to
> register more property files.  Right now, that's not possible
> (PropertyFileConfig is only read once, and only via ServiceLoader).
>
> Maybe thats the better discussion to have.
>
>
>
>>  Just trying to make a few steps back to look at the problem from a
>>  distance...
>>
>>  LieGrue,
>>  Strub
>>
>>  > Am 21.08.2016 um 16:11 schrieb Romain Manni-Bucau
> <[hidden email]
>>  >:
>>  >
>>  > 2016-08-21 16:08 GMT+02:00 Mark Struberg
> <[hidden email]>:
>>  >
>>  >> EnvironmentPropertyConfigSourceProvider is effectively an internal
>>  class.
>>  >> And thus I'd not move it to the API. Just look how much we
> would need to
>>  >> add to the api package.
>>  >>
>>  >> I understand that you don't like to duplicate code.
>>  >>
>>  >> Usually things like ConfigSources are totally orthogonal to your
>>  >> production code. So you need those only as runtime dependency.
>>  > I get the idea but this is likely not what you wanted to say.
> Typically
>>  > sources are production code (but not business code).
>>  >
>>  >
>>  >>
>>  >> In that case just make a tool module which has ds-core-impl as
> compile
>>  >> time dependency and then you can simply re-use and even extend the
>>  internal
>>  >> class.
>>  >> But that class is clearly NOT an API class imo.
>>  > Question is: how stable do we consider -impl is? If the solution to
> that
>>  > issue is 1. make the class (whichever it is) public 2. say to the user
> to
>>  > import -impl in his pom, then we need to show that the API is stable
> and
>>  > not just an internal we can break when we want. @Stable would be an
>>  option
>>  > but moving it to -api is another one. Don't care of the solution
> while we
>>  > prevent the user to duplicate our -impl code.
>>  >
>>  >
>>  >> LieGrue,
>>  >> strub
>>  >>
>>  >>
>>  >>
>>  >>
>>  >>
>>  >>> On Sunday, 21 August 2016, 15:28, John D. Ament
> <[hidden email]
>>  >
>>  >> wrote:
>>  >>>> Hi,
>>  >>>
>>  >>> I have a use case for leveraging something
>>  >>> like EnvironmentPropertyConfigSourceProvider to load multiple
> property
>>  >>> files, outside of apache-deltaspike.properties.  Right now its
> package
>>  >>> local and in the impl JAR.  I'd rather not duplicate the
> logic since
>>  >> whats
>>  >>> being done is exactly what is needed.  I was wondering if
> anyone had
>>  any
>>  >>> concerns with making this an API class instead of an impl
> class?
>>  >>>
>>  >>> John
>>  >>
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: Increased visibility of EnvironmentPropertyConfigSourceProvider?

Romain Manni-Bucau
Think we need to:

1. make impl shorter providing few default impl base - mainly ones from
impl module ensuring there are public and "@Stable"
2. Support more cdi captures - not only PropertyFileSource even if limited
as you pointed out (works for app which config the extensions in embedded
files ~= internal app config vs external envrt values)

Wdyt?

Le 21 août 2016 21:48, "Mark Struberg" <[hidden email]> a écrit :
>
> Thanks for adding a link to the config stuff in a more visible place!
>
> Yes, I also think we should have those classes public (but in the impl
module).
>
> Regarding the PropertyFileConfig:
> I think I documented this, but better mention it again:
> You can register it in 2 ways
>
> 1.) Simply implement the interface
>  -> in that case it gets picked up via ProcessAnnotatedType and will
_only_ be available after the container did finish booting!
>  That means that you cannot use values from those ConfigSources during
CDI boot, e.g. for @Exclude
>
> 2.) To work around the timing issue in 1 we later also made it pick up
via java.util.ServiceLoader. In that case it will get picked up like a
regular ConfigSource and is also available during container bootstrap.
>
> Just don't fortet to add a @Exclude annotation to prevent the
PropertyFileConfig from being picked up twice.
>
>
> Happy to get any feedback about how to improve the docs if something is
missing.
>
>
> LieGrue,
> strub
>
>
>
>
> > On Sunday, 21 August 2016, 20:57, John D. Ament <[hidden email]>
wrote:
> > > On Sun, Aug 21, 2016 at 2:44 PM Mark Struberg
<[hidden email]>
> > wrote:
> >
> >>  Oh if it's not public then we should really make it!
> >>
> >>  You are right with 'production code'. Of course it's in
> > production.
> >>  But it's more like infrastructure glue code and not business related,
> >>  right?
> >>  Means your colleagues tinkering on UI and business should typically
not
> >>  even get it as compile scoped dependency but really only as runtime
dep.
> >>  John, romain, do you agree on this point?
> >>
> >
> > Right, the problem is that the class isn't even public.  So even if I
> > declare a proper dependency on it, can't be used.  I would say my view
> > point is that its SPI, along with a few other classes (MapConfigSource,
> > PropertiesConfigSource, PropertyFileConfigSource).  Realistically
though,
> > the problem I was trying to solve was resolved by implementing
> > "PropertyFileConfig" but trying to remember how that should work, I
> > couldn't even find the doc page (eventually found it, its mentioned
briefly
> > on core, but really should be its own link on the landing page, which I
> > changed).
> >
> > I could also see the class not being SPI.  As long as things are
configured

> > properly, its not needed.  That's assuming we have a dynamic way to
> > register more property files.  Right now, that's not possible
> > (PropertyFileConfig is only read once, and only via ServiceLoader).
> >
> > Maybe thats the better discussion to have.
> >
> >
> >
> >>  Just trying to make a few steps back to look at the problem from a
> >>  distance...
> >>
> >>  LieGrue,
> >>  Strub
> >>
> >>  > Am 21.08.2016 um 16:11 schrieb Romain Manni-Bucau
> > <[hidden email]
> >>  >:
> >>  >
> >>  > 2016-08-21 16:08 GMT+02:00 Mark Struberg
> > <[hidden email]>:
> >>  >
> >>  >> EnvironmentPropertyConfigSourceProvider is effectively an internal
> >>  class.
> >>  >> And thus I'd not move it to the API. Just look how much we
> > would need to
> >>  >> add to the api package.
> >>  >>
> >>  >> I understand that you don't like to duplicate code.
> >>  >>
> >>  >> Usually things like ConfigSources are totally orthogonal to your
> >>  >> production code. So you need those only as runtime dependency.
> >>  > I get the idea but this is likely not what you wanted to say.
> > Typically
> >>  > sources are production code (but not business code).
> >>  >
> >>  >
> >>  >>
> >>  >> In that case just make a tool module which has ds-core-impl as
> > compile
> >>  >> time dependency and then you can simply re-use and even extend the
> >>  internal
> >>  >> class.
> >>  >> But that class is clearly NOT an API class imo.
> >>  > Question is: how stable do we consider -impl is? If the solution to
> > that
> >>  > issue is 1. make the class (whichever it is) public 2. say to the
user

> > to
> >>  > import -impl in his pom, then we need to show that the API is stable
> > and
> >>  > not just an internal we can break when we want. @Stable would be an
> >>  option
> >>  > but moving it to -api is another one. Don't care of the solution
> > while we
> >>  > prevent the user to duplicate our -impl code.
> >>  >
> >>  >
> >>  >> LieGrue,
> >>  >> strub
> >>  >>
> >>  >>
> >>  >>
> >>  >>
> >>  >>
> >>  >>> On Sunday, 21 August 2016, 15:28, John D. Ament
> > <[hidden email]
> >>  >
> >>  >> wrote:
> >>  >>>> Hi,
> >>  >>>
> >>  >>> I have a use case for leveraging something
> >>  >>> like EnvironmentPropertyConfigSourceProvider to load multiple
> > property
> >>  >>> files, outside of apache-deltaspike.properties.  Right now its
> > package
> >>  >>> local and in the impl JAR.  I'd rather not duplicate the
> > logic since
> >>  >> whats
> >>  >>> being done is exactly what is needed.  I was wondering if
> > anyone had
> >>  any
> >>  >>> concerns with making this an API class instead of an impl
> > class?
> >>  >>>
> >>  >>> John
> >>  >>
> >>
> >>
> >