#418 config profile support #554
Conversation
| ---- | ||
| @Inject @ConfigProperty(name="vehicle.name") String vechileName; | ||
| ---- | ||
|
|
There was a problem hiding this comment.
We need some normative text covering what conforming implementations are required to do.
There was a problem hiding this comment.
Detailed description of resolution rules and interplay between ordinals and other config sources are needed here and these rules also tested within the TCK.
There was a problem hiding this comment.
+1 @smillidge , I will add more TCKs for the config source ordinal rules once we are happy with the rules. See here for more explaination.
|
I am working on TCKs. Please continue to comment on the behaviour. |
Emily-Jiang
force-pushed
the
config-profile-issue
branch
from
bc8e7c9 to
8583c26
Compare
| Config Profile indicates the project phase, such as dev, testing, live, etc. | ||
| === Specify Config Profile | ||
|
|
||
| The config profile can be specified via a system property `mp.config.profile`. It can be set when starting your microservice, e.g. |
There was a problem hiding this comment.
We should also allow builders to set the profile for the Config they are building. Using a system property is nice on a global level but it doesn't make sense if an application uses a custom config or even more custom configs.
A new method like this in ConfigBuilder would allow it:
ConfigBuilder withProfile(String profileName)
There was a problem hiding this comment.
It ought to be possible to set the config profile via a configuration property IMO.
There was a problem hiding this comment.
Since when reading config sources, we need to figure out the config profile value, using system properties is much simplier. What will happen if set different value for mp.config.profile in different config sources, which makes the situation very complicated.
There was a problem hiding this comment.
We could also allow setting the profile using env var. But setting it via a config property introduces recursion, adds too many places where it can be set which can make the configuration very confusing.
Implementations can still support additional unspecified ways to set the profile.
There was a problem hiding this comment.
Highest config source wins, as usual. We do support self-configuration in other areas, so I don't think this is so unusual.
| * Set the config profile. | ||
| * Once the config profile is set, the config property {@code %profile.config.name} overrides the property {@code config.name} | ||
| * in the same config source. | ||
| * The config value for the property {@code config.name} is retrieved from a config source with the highest ordinal. | ||
| * The value is set by the property {@code %profile.config.name} if exists. Otherwise, the value is taken from the property {@code config.name}. | ||
| * @param profile the config profile | ||
| * @return |
There was a problem hiding this comment.
| * Set the config profile. | |
| * Once the config profile is set, the config property {@code %profile.config.name} overrides the property {@code config.name} | |
| * in the same config source. | |
| * The config value for the property {@code config.name} is retrieved from a config source with the highest ordinal. | |
| * The value is set by the property {@code %profile.config.name} if exists. Otherwise, the value is taken from the property {@code config.name}. | |
| * @param profile the config profile | |
| * @return | |
| * Set the configuration profile. | |
| * Once the configuration profile is set, the configuration property {@code %profile.config.name} overrides the property {@code config.name} | |
| * in the same configuration source. | |
| * The configuration value for the property {@code config.name} is retrieved from a configuration source with the highest ordinal. | |
| * The value is set by the property {@code %profile.config.name} if exists. Otherwise, the value is taken from the property {@code config.name}. | |
| * @param profile the configuration profile name (must not be {@code null}) | |
| * @return this builder |
Also the resolution of #553 would affect the wording here.
There was a problem hiding this comment.
When I tried to add tests for this spi, I suggest against the idea adding this method, because it will create more confusions. Setting mp.config.profile is better set via the config sources to avoid unnecessary conflicts of setting profile via a method or via a property.
api/src/main/java/org/eclipse/microprofile/config/spi/ConfigBuilder.java
Show resolved
Hide resolved
| Config Profile indicates the project phase, such as dev, testing, live, etc. | ||
| === Specify Config Profile | ||
|
|
||
| The config profile can be specified via a system property `mp.config.profile`. It can be set when starting your microservice, e.g. |
There was a problem hiding this comment.
We could also allow setting the profile using env var. But setting it via a config property introduces recursion, adds too many places where it can be set which can make the configuration very confusing.
Implementations can still support additional unspecified ways to set the profile.
tck/src/main/java/org/eclipse/microprofile/config/tck/profile/DevConfigProfileTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Emily Jiang <[email protected]>
Emily-Jiang
force-pushed
the
config-profile-issue
branch
from
1ce434f to
e1d5ef2
Compare
… into config-profile-issue
Signed-off-by: Emily Jiang <[email protected]>
| java -jar myapp.jar -Dmp.config.profile=testing | ||
| ---- | ||
|
|
||
| The property `mp.config.profile` is only read once on microservice starting. Any changes after that is ignored. Furthermore, the value specifies a single profile. Only one profile can be active at a time. |
There was a problem hiding this comment.
| The property `mp.config.profile` is only read once on microservice starting. Any changes after that is ignored. Furthermore, the value specifies a single profile. Only one profile can be active at a time. | |
| The property `mp.config.profile` shouldn't change after the JVM is started. It's only read once during JVM start up. If the property is modified afterwards the behavior is undefined and any changes to its value made later can be ignored by the implementation. Furthermore, the value specifies a single profile. Only one profile can be active at a time. |
We shouldn't refer to "microservices" which is a vague term and not all MP implementations are used to build microservices. Rather I'd refer to a running JVM instead of a microservice.
There was a problem hiding this comment.
I had a further thought on this. I am going to change to application starting instead of JVM startup as it is more accurate to say application starting due to some application server might have multiple applications installed on the same server.
There was a problem hiding this comment.
How about on Config creation?
There was a problem hiding this comment.
How about on Config creation?
This will be from implementation point of view though, which is different from user perspective. Some implementations might create the config a lot earlier.
There was a problem hiding this comment.
We can all agree that the profile is read one time. But it doesn't make sense to specify exactly when it must be read; all we can really say for sure is that the implementation will (of a necessity) read the profile property before the application reads or injects configuration properties. Since that's all the spec can reasonably require, that's exactly what should be specified. Obviously this doesn't happen during JVM startup; it might not happen during application startup either.
tck/src/main/java/org/eclipse/microprofile/config/tck/profile/DevConfigProfileTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Emily Jiang <[email protected]>
Signed-off-by: Emily Jiang <[email protected]>
|
@OndroMih we talked about setting |
Signed-off-by: Emily Jiang <[email protected]>
|
We talked about supporting
The reasoning is that we don't need it to implement tests and we don't yet have a usecase that would justify standardizing this. I suggest that we initially approve that |
I'm sorry, but I strongly disagree with this. This is not about requiring the property for testing or anything else, is about us being consistent with the spec we are proposing. We have a spec that is all about Configuration and we try to convince our users to use it and have their own ConfigSources and manage their configuration. When we need to set our own configuration we are not even able to follow our own spec. How can we possibly justif to an end user that we are not able to support this? |
|
Well then you are going to have to properly specify what occurs in a runtime that supports multiple deployments. What happens if the system properties config source has one value for profile. Application A has another and Application B another. Which profile value takes precedence? I assume that it is the ConfigSource with the higher Ordinal value i.e. System Properties? |
|
We follow the exact same rules as we have specified for loading configurations. Forget about profile for a second. In that scenario, if I have |
| assertThat(ConfigProvider.getConfig().getValue("vehicle.name", String.class), is(equalTo("bus"))); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank line.
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank lines.
| */ | ||
| public class TestConfigProfileTest extends Arquillian { | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank line.
|
|
||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank line.
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank lines.
| */ | ||
| public class TestCustomConfigProfile extends Arquillian { | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank line.
|
|
||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank line.
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank lines.
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank lines.
|
|
||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank line.
| assertThat(ConfigProvider.getConfig().getValue("vehicle.name", String.class), is(equalTo("bike"))); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank line.
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank lines.
| */ | ||
| public class InvalidConfigProfileTest extends Arquillian { | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank line.
|
|
||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank line.
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank lines.
|
|
||
|
|
There was a problem hiding this comment.
Remove additional blank lines.
Signed-off-by: Emily Jiang <[email protected]>
|
@radcortez thanks for your feedback! I've addressed all of your comments, I think. Please double check and approve or fill more comments! |
|
Thanks! +1 |
|
@dmlloyd @OndroMih @struberg in the past you requested some changes. I believed I have either addressed your comments or responded to your comments with reasons if your suggestion did not result into a update. I plan to merge this PR this Thursday EOD UK time as @radcortez has approved this PR. Please either approve the PR or fill your objection from an end user point of view with a detailed use case before EOD Thursday. I take silence as no objection. Thanks! |
Emily-Jiang
changed the title
|
Hi @Emily-Jiang, I'd be grateful if you allowed for more than 2 days to react. I don't work 100% on MicroProfile and there are also otehr PRs open that it's very easy to miss a 2 day deadline. I only noticed your deadline after you'd merged this PR actually. @struberg gave a very reasonable feedback a few days ago, which I'd like to discuss further. When I review the discussion, most of the time it's just @struberg's or my opinion against @radcortez or @dmlloyd, both from the Quarkus team. So I don't understand why proposals from the Quarkus team always win without further discussion. Regarding support for setting the profile in any config source, I'm now OK with that since most of the people seem to support it or at least aren't against. Since that was the most important unresolved issue for me I'm OK with merging this PR. But I'm not OK with how it was finally merged with just an approval by a single person and with the lack of time to react before merging it. |
|
@OndroMih this PR has been open for about 2 months. I have noted on the gitter and ask for feedback. Both @jbee and @radcortez were happy with the changes at today's hangout. By the way, you can always raise further issues if you are unhappy with any issues. I talked with @struberg regarding the postfix and prefix and eventually he was ok with the changes and he also supports the profile property specified in any config sources. I also want to say it is not done deal and you can always raise further issues. However, we need to move forward. |
|
Hi, @OndroMih I understand your frustration, but please don't feel that your opinion is being disregarded. If that happened, I'm sure everyone else is willing to review the changes and make any adjustments. Also, I would like to tell you that I have a tremendous amount of respect for you and I value your opinions (and everyone else for that matter in this group). I hope I can convince you that this is not a Quarkus team vs everyone else. To make it clear, I'm in the SmallRye team, which provides the Config implementation not only to Quarkus, but also to Wildlfy, JBoss EAP, Thorntail, and now Websphere, so Quarkus is not my only concern. For instance, I advocate that the config property for the profile to follow the same rules as everything else. Quarkus doesn't support that and works in the same way you were proposing. On the other hand, I felt that this is was not consistent with our spec, so I've pushed back. In the end, Quarkus is probably going to have a similar issue with yours on how to implement it. In the ConfigValue discussion, I've proposed a solution, you reviewed and made your proposal. I've updated the PR and it now reflects more of your proposal than mine. And that is fine because it made sense and we were able to collaborate on that. I hope that you realize that I'm here to help and not to follow an agenda. Of course, we are not always going to agree and that is ok. As long as we can move forward its all good. |
|
Thanks for clarification, @radcortez and @Emily-Jiang. I respect both of you deeply but I was afraid that we're moving too fast with this because yhis is a huge change and the PR is huge by itself. I'm very busy these days and I can hardly find time to review one PR a week so having 3 or more PRs open at once is challanging to follow. I'd appreciate your understanding and give longer notice before merging. Since so many people eventually agreed with the final version of this PR, I'm OK with metging it although I don't fully agree with it. Especially my concerns about configuring the config by config itself may cause headaches to implementers and may also be confusing to users without much added value. Before we release the next version of Config I'd like to have a POC impl by both Quarkus and some app server, e. g. Payara, OpenLiberty or WildFly |
|
And BTW, I like the final version of the ConfigValue PR. Thanks, @radcortez. I believe we can still build on that and add additional capability to ConfigValue for more usecases. |
|
Hey @OndroMih. I've already implemented this in SmallRye Config. If you want to have a look on how it is done, check here: https://github.com/smallrye/smallrye-config/blob/master/implementation/src/main/java/io/smallrye/config/ProfileConfigSourceInterceptor.java |
A proposal to support standard way to provide profile-specific properties that would be used only if a profile is specified. This for example enables an application to include configuration for different environments and development stages while only one of them is active (e.g. dev, test, prod).