Vote is cancelled, we also still miss a binding +1 LieGrue, strub
> Am 08.08.2017 um 11:40 schrieb Mark Struberg <[email protected]>: > > I think the API is quite fine. What are your concrete concerns? > > There are btw 2 reasons why the name in the @ConfigProperty is @Nonbinding: > > a.) you would need to register about 2000 different Beans otherwise (one per > injection point basically > b.) the default naming (name="") would break it anyway. You again would need > to create dynamic qualifiers for each of those injection points. > > LieGrue, > strub > >> Am 08.08.2017 um 11:34 schrieb Romain Manni-Bucau <[email protected]>: >> >> we can rollback it, anyway I was not that involved in MP but any hope the >> spec change enough to make the runtime pipeline straight forward and avoid >> any resolution at that time? Don't think the API is strong ATM >> >> >> Romain Manni-Bucau >> @rmannibucau | Blog | Old Blog | Github | LinkedIn | JavaEE Factory >> >> 2017-08-08 11:33 GMT+02:00 Mark Struberg <[email protected]>: >> Had a talk with Romain via IRC. His goal was to improve the performance >> during injection by providing separate Beans for each Injection Point. >> We debugged through and in practice the benefit only pays off if you have >> just a single @ConfigProperty Provider<T> for each type T. This will imo >> merely increase the performance for very simple apps and samples. So while >> the goal is good it' imo not worth it as it will not affect real world >> projects. In those you will almost always have multiple Provider<String> for >> example. >> >> It was worth a try, but imo the increased complexity (from 50 LOC to 400 >> LOC) isn't worth the benefits which only will affect very simple apps anyway. >> >> Should we go on and ship this version anyway and rework later, or just do it >> and qickly re-roll (will not take long)? >> >> LieGrue, >> strub >> >>> Am 08.08.2017 um 09:24 schrieb Mark Struberg <[email protected]>: >>> >>> Yes that's something else I saw yesterday as well. >>> Gonna fix that and reroll. >>> >>> LieGrue, >>> strub >>> >>>> Am 08.08.2017 um 01:24 schrieb John D. Ament <[email protected]>: >>>> >>>> Agreed that it would make sense that this required check is skipped for >>>> provider, optional. Only Optional is called out though. >>>> >>>> At the same time, I think we're over eagerly saying that fields are not >>>> set. I raised a spec issue yesterday, basically if the value is >>>> "org.eclipse.microprofile.config.configproperty.unconfigureddvalue" that >>>> seems to be what indicates its not set. However, GConfig is also blowing >>>> up when it's set to "". >>>> >>>> I don't see an issue fixing this on master. However, right now I have >>>> master targetting config 1.1. We can revert back to 1.0 to cut a 1.1 >>>> shortly. >>>> >>>> John >>>> >>>> On Mon, Aug 7, 2017 at 7:16 PM Mark Struberg <[email protected]> wrote: >>>> It's a requirement for direct injection, but not for Provider<T> afair >>>> >>>> Lg, >>>> Strub >>>> >>>> Am 08.08.2017 um 01:03 schrieb John D. Ament <[email protected]>: >>>> >>>>> >>>>> I took a look at the test and the commit, to remember why this would fail. >>>>> >>>>> The test fails because the property isn't set. This is actually a MP >>>>> Config requirement, validating injection points. If I add >>>>> System.setProperty(SOME_KEY, "no-op"); to the beginning of the deployment >>>>> method, it works as expected. See [1] for the API requirements >>>>> >>>>> Now that test continues to fail on Weld3 with this change in, seemingly >>>>> the custom provider doesn't work. Are you using Weld or OWB as your >>>>> runtime? >>>>> >>>>> [1]: >>>>> https://github.com/eclipse/microprofile-config/blob/1.0/api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java#L50 >>>>> >>>>> >>>>> On Mon, Aug 7, 2017 at 5:27 PM Mark Struberg <[email protected]> wrote: >>>>> I have now tried to use this version in a project and totally blew up. >>>>> I reviewed the code in depth and I'm not sure how to fix it except with a >>>>> revert of some committs. >>>>> >>>>> So I gonna change my vote to -1 >>>>> >>>>> The reason seems to be r1800744 which imo introduced uneccessary >>>>> complexity and broke quite a few features. >>>>> >>>>> e.g. injection of Provider is now broken. >>>>> It also breaks programmatic lookup, etc. >>>>> >>>>> How to proceed? >>>>> >>>>> LieGrue, >>>>> strub >>>>> >>>>> >>>>>> Am 01.08.2017 um 21:54 schrieb Mark Struberg <[email protected]>: >>>>>> >>>>>> And my own +1 of course. >>>>>> >>>>>> LieGrue, >>>>>> strub >>>>>> >>>>>> >>>>>>> Am 01.08.2017 um 11:16 schrieb Reinhard Sandtner >>>>>>> <[email protected]>: >>>>>>> >>>>>>> +1 (non-binding) >>>>>>> >>>>>>> lg >>>>>>> reini >>>>>>> >>>>>>>> Am 30.07.2017 um 12:16 schrieb Jean-Baptiste Onofré >>>>>>>> <[email protected]>: >>>>>>>> >>>>>>>> +1 (non binding) >>>>>>>> >>>>>>>> Regards >>>>>>>> JB >>>>>>>> On Jul 30, 2017, at 12:04, Jean-Louis MONTEIRO <[email protected]> >>>>>>>> wrote: >>>>>>>> +1 >>>>>>>> Thanks >>>>>>>> >>>>>>>> Le ven. 28 juil. 2017 à 13:20, Romain Manni-Bucau >>>>>>>> <[email protected]> a écrit : >>>>>>>> +1 (for the release and the src zip in dist) >>>>>>>> >>>>>>>> >>>>>>>> Romain Manni-Bucau >>>>>>>> @rmannibucau | Blog | Old Blog | Github | LinkedIn | JavaEE Factory >>>>>>>> >>>>>>>> 2017-07-28 14:17 GMT+02:00 Mark Struberg <[email protected]>: >>>>>>>> Yes we should make sure this gets propagated to dist! >>>>>>>> >>>>>>>> LieGrue, >>>>>>>> strub >>>>>>>> >>>>>>>>> Am 28.07.2017 um 13:58 schrieb John D. Ament <[email protected]>: >>>>>>>>> >>>>>>>>> +1 to ship it. >>>>>>>>> >>>>>>>>> One comment, there's been input in the past that geronimo releases >>>>>>>>> don't show up in the system. Should we ensure that the final result >>>>>>>>> gets put into https://dist.apache.org/repos/dist/release/geronimo/ ? >>>>>>>>> >>>>>>>>> On Fri, Jul 28, 2017 at 7:25 AM Mark Struberg <[email protected]> >>>>>>>>> wrote: >>>>>>>>> Hi! >>>>>>>>> >>>>>>>>> Apache geronimo-config is an implementation of the Microprofile-1.0 >>>>>>>>> Config specification [1][2]. >>>>>>>>> >>>>>>>>> It allows flexible and extensible Configuration for applications. >>>>>>>>> >>>>>>>>> >>>>>>>>> Here is our staging repo >>>>>>>>> https://repository.apache.org/content/repositories/orgapachegeronimo-1035/ >>>>>>>>> >>>>>>>>> The Source distribution can be found here: >>>>>>>>> https://repository.apache.org/content/repositories/orgapachegeronimo-1035/org/apache/geronimo/config/config-parent/1.0/ >>>>>>>>> >>>>>>>>> >>>>>>>>> Our own tag in SVN is >>>>>>>>> https://svn.apache.org/repos/asf/geronimo/components/config/tags/config-parent-1.0/ >>>>>>>>> >>>>>>>>> >>>>>>>>> Please VOTE >>>>>>>>> [+1] yeah, ship it! >>>>>>>>> [+0] meh, don't care >>>>>>>>> [-1] nope, because ${showstopper} >>>>>>>>> >>>>>>>>> The VOTE is open for 72h >>>>>>>>> >>>>>>>>> txs and LieGrue, >>>>>>>>> strub >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> [1] https://github.com/eclipse/microprofile-config >>>>>>>>> [2] https://github.com/eclipse/microprofile-config/releases >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>> >> >> >
