eclipse / microprofile-config

MicroProfile Configuration Feature
Apache License 2.0
213 stars 115 forks source link

Clarify how source ordinal applies with an active profile #782

Closed poikilotherm closed 1 year ago

poikilotherm commented 1 year ago

Closes #781

This pull request tries to rephrase parts of the profile on property level spec to clarify how ordinal of sources and the profiled vs. unprofiled values work together.

eclipse-microprofile-bot commented 1 year ago

Can one of the admins verify this patch?

Emily-Jiang commented 1 year ago

@poikilotherm you need to sign ECA form first. Please click on the Details above and follow instructions there.

poikilotherm commented 1 year ago

@poikilotherm you need to sign ECA form first. Please click on the Details above and follow instructions there.

Was already on it :wink: Successfully signed and re-validated just now.

Please let me know what you think about the extended spec. Thank you very much for looking into this @Emily-Jiang !

Emily-Jiang commented 1 year ago

Thank you @poikilotherm for the PR! Can you also add a couple of sentence on the spec to state the behavior?

poikilotherm commented 1 year ago

Thank you @poikilotherm for the PR! Can you also add a couple of sentence on the spec to state the behavior?

Hi @Emily-Jiang thanks for the feedback. Unfortunately, I'm not sure what you are referring to? Is there some other place in the spec that I should edit besides the profile details that needed clarification? Any hints are much appreciated!

Emily-Jiang commented 1 year ago

Thank you @poikilotherm for the PR! Can you also add a couple of sentence on the spec to state the behavior?

Hi @Emily-Jiang thanks for the feedback. Unfortunately, I'm not sure what you are referring to? Is there some other place in the spec that I should edit besides the profile details that needed clarification? Any hints are much appreciated!

Sorry for not being clear. I have added more detailed thoughts inline. Will you be able to take at this PR?

poikilotherm commented 1 year ago

@Emily-Jiang thank you for your additional feedback!

I rephrased a little as you suggested. Please let me know if this is better now.

I already worked on adding a new TCK test locally, but struggeling to find a property not altered by other tests, so it can be used here. Will continue coding, maybe push something preliminary, too, so y'all could have a look.

poikilotherm commented 1 year ago

@Emily-Jiang @radcortez @smillidge I just added the new TCK test to this PR.

If you want to see it in action, I took notes how to execute the test in addition to the others: https://notes.desy.de/iDjhL8UESlO-_RGNKLDRZQ

As expected, Payara fails to be spec compliant, while SmallRye is compliant.

Emily-Jiang commented 1 year ago

@poikilotherm can you address the above comments so that I can merge your PR?

poikilotherm commented 1 year ago

@poikilotherm can you address the above comments so that I can merge your PR?

Please excuse my slow response time @Emily-Jiang - I'm on vacation. Applied changes as requested. Thank you again for looking into this!

Emily-Jiang commented 1 year ago

@poikilotherm Can you fix the following format issue?

The GHA failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.22.0:validate (default-cli) on project microprofile-config-tck: File '/home/runner/work/microprofile-config/microprofile-config/tck/src/main/java/org/eclipse/microprofile/config/tck/profile/OverrideConfigProfileTest.java' has not been previously formatted. Please format file (for example by invoking mvn -f tck net.revelc.code.formatter:formatter-maven-plugin:2.22.0:format) and commit before running validation!

poikilotherm commented 1 year ago

@Emily-Jiang done! :smile:

Emily-Jiang commented 1 year ago

I will squash and merge your PR @poikilotherm and deal with the format with a separate PR if the build still fails.

poikilotherm commented 1 year ago

Thank you very much @Emily-Jiang !

Much appreciated! Glad this is fixed now.