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.

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

Create a CDI api for Microprofile Config #39

Closed
smillidge opened this issue Jan 26, 2017 · 18 comments
Closed

Create a CDI api for Microprofile Config #39

smillidge opened this issue Jan 26, 2017 · 18 comments
Milestone

Comments

@smillidge
Copy link
Contributor

smillidge commented Jan 26, 2017

I'm creating the CDI api issue as IMHO this is a requirement for a 1.0 drop of microprofile-config. Ideally discussion about CDI can be associated with this issue.

One of the 3 core apis of microprofile and a microprofile runtime is the provision of CDI. To provide a standard programming model for developers using MIcroprofile Config needs to provide a binding to CDI.

@OndroMih
Copy link
Contributor

Linking the discussion in another PR.

@OndroMih
Copy link
Contributor

Some people suggested to target JSR330 instead of CDI, to get wider adoption even outside of MP and make things simple. Sounds fine to me to target JSR330 compatibility.
It should be easy to do on the API consumer side, with plain @Inject and qualifiers. However, I'm not sure if JSR330 would be enough for an SPI, if we want to provide API to provide a custom config source. We'll see, but we don't have to have the CDI-based SPI in v1. I'll propose a CDI based API inspired by the Sabot project (by Tomitribe) soon, targeting only JSR330 if possible.

@smillidge
Copy link
Contributor Author

I think we should leverage the power of full CDI rather than just @Inject because CDI is available in Microprofile.

@struberg
Copy link
Contributor

struberg commented Jan 26, 2017

while others are talking... #40

;-)

@keilw
Copy link
Contributor

keilw commented Jan 26, 2017

@smillidge @OndrejM @Emily-Jiang Could you check out that PR, guess all of you could push/merge it if it meets what we had in mind for DI support?

Good too see it also seems to tackle #34 instead of talking against it for days ;-)

@struberg
Copy link
Contributor

This PR is up for discussion for now. I can merge it myself if all are OK with it.
It's also just wip. Will commit other stuff soon.

@keilw
Copy link
Contributor

keilw commented Jan 26, 2017

We cannot use Gerrit right now, but even if one merges their own PR it would be good to have some form of approval, either a "+1" or something similar.

@struberg
Copy link
Contributor

That's why I did not yet push it...

@smillidge
Copy link
Contributor Author

You can configure Github to require review and approval before merge.

@struberg
Copy link
Contributor

@smillidge the point is not the formal approval as I have commit rights. The point is educated feedback. This is now as CDI as it probably gets!
Of course we still need to add injection on top of that. But that is additional sugar once the core mechanism works. We can add all those later. What I would love to get now is that someone sits down and tries to understand what Romain and I did here. Happy to explain on a hangout as well if needed.

@keilw
Copy link
Contributor

keilw commented Jan 26, 2017

Guess it depends on what the Microprofile team (and its project leads) want. Not sure if it's mandatory, but a large number of Apache projects also have PMC votes before substantial commits or changes. https://projects.eclipse.org/projects/technology.microprofile/who is what seems the "PMC" of Microprofile right now. Btw. I trust everyone who has commit rights here does have a valid Eclipse committer and all agreements signed and up to date? Otherwise it could get tricky for such commits, regardless of being approved or not. @waynebeaton can probably tell more on that, he pointed to this https://www.eclipse.org/projects/handbook/#resources-commit in the mailing list a while ago.

Romain could be fine as he works for Tomitribe, but as I am Individual committer, I am not sure, what is required for employees.

@keilw
Copy link
Contributor

keilw commented Jan 27, 2017

Have to look at it in a little more detail. I saw at least 2 participants in either mailing list, hangout or both reviewed the PR. Guess that is how we might like to do this for most changes, regardless of who does the push in the end :-) @struberg Which of the 3 parts of CDI 2 does it use? http://docs.jboss.org/cdi/spec/2.0.Beta1/cdi-spec.html#doc_organisation The main reason is not to run it in SE or on a desktop, but in a cloud environment, where JAR-sizes may cost a lot of money to avoid using the Full Java EE scope for Microprofile ;-) One of the good things of this not a spec is, we could use CDI 2 soon after it's final.

@Emily-Jiang
Copy link
Member

Was on another mission this week. I have a number of issues with this PR of the CDI approach.

@smillidge
Copy link
Contributor Author

I can take a look at the weekend as well. I'd like to see the ability to add direct injection of config values but that can be added on top of the base PR.

@struberg
Copy link
Contributor

Was there any pull request for this ticket besides my 'cdi-only' proof of concept?

I know @OndrejM started in something #86

It this the very ticket we should use for it?

@OndroMih
Copy link
Contributor

Currently, we have these PRs for proposals/POCs:

According to the hangout yesterday, we are now considering either #42 or #86 (and also whether #86 can support Provider/Instance injection to provide the same dynamic features as #42)

@OndroMih OndroMih added this to the config-1.0 milestone Mar 10, 2017
@struberg
Copy link
Contributor

We have quite a few different branches open which have lots of overlap. So I think it would be good to work on those together to move them forward.

@struberg
Copy link
Contributor

status update:

Closing this ticket.
Please speak out if you think something is still missing - txs!

@dmlloyd dmlloyd modified the milestones: config-1.0, MP Config 1.0 Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants