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

Remove YAML shortcomings section as it no longer applies #24620

Closed
wants to merge 2 commits into from

Conversation

@friscoMad
Copy link
Contributor

@friscoMad friscoMad commented Dec 29, 2020

This is a small documentation fix, I was working today with multi-document profile-specific YAML files and after reading the example I thought that directly that setup does not work, but that's not the case it is just the example is not correct.

The current example does work as expected as the active profile is the same as the one in the profile-specific file so the nested document is not filtered out, the file name or the active profile should be changed to make the filter out the nested document.

Anyway, that whole part and example probably should be moved to section 2.3.8 as it is no longer exclusive of Yaml files, it is related to multi-document files.

That whole part and example should be moved to another area as it is no longer exclusive of Yaml files, it is related to multi-document files.
Anyway, the current example does work as expected as the active profile is the same as the one in the profile-specific file and the nested document is not filtered out.
@pivotal-issuemaster
Copy link

@pivotal-issuemaster pivotal-issuemaster commented Dec 29, 2020

@friscoMad Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@pivotal-issuemaster pivotal-issuemaster commented Dec 29, 2020

@friscoMad Thank you for signing the Contributor License Agreement!

@mbhave
Copy link
Contributor

@mbhave mbhave commented Jan 4, 2021

@friscoMad I don't think I understand the reason for changing the profile name to prod. That bit of documentation is trying to say that a profile-specific files (in this case, application-dev.yml that is loaded when the dev profile is active) cannot be used in conjunction with spring.config.activate.on-profile.

However, I do agree that this section is true for any multi-document files and not just YAML. Would you like to update your PR to reflect that?

@snicoll
Copy link
Member

@snicoll snicoll commented Jan 19, 2021

@friscoMad how is it going? Can you please let us know if you have the time to review the proposal as @mbhave indicated? Thanks!

@friscoMad
Copy link
Contributor Author

@friscoMad friscoMad commented Jan 19, 2021

@snicoll @mbhave Sorry I had a week off and forgot about this PR.
I will try to update the PR to reflect that this common for yaml and properties but that will leave the yaml shortcomings section with just the @PropertySource warning.
But I need to be sure what was the intention of the example we are talking about, my understanding is that in the example we want to show a nested document with an activation condition that is true but the parent is a profile-specific file and can be fully skipped if the profile is not active.

In the current example the parent depends on dev and the child on !test but we are also saying that we run it with --spring.profiles.active=dev, if that's the case the file is processed and the nested document will be loaded as test is not enabled.
My proposal was changing the command line part to prod but we can just say that the mypassword will be set to "secret" if and only if the dev profile is active and test is not active. So that nested documents conditions in profile-specific config files have & parent-profile implicitly added to the activation condition.

@mbhave
Copy link
Contributor

@mbhave mbhave commented Feb 16, 2021

@friscoMad The example is trying to show unexpected behavior when there is a nested document in a profile-specific file. It is saying that even if you have the dev profile activated and the application-dev.yml file is loaded, the nested document under !test will not be processed. This is due to the filtering of nested documents that occurs in profile-specific files. It is an example to show why you should not use spring.config.activate.on-profile with profile-specific files. Hope this answers your question.

@mbhave
Copy link
Contributor

@mbhave mbhave commented Feb 16, 2021

@friscoMad My comment above is no longer valid since the behavior has changed in #24990. This change allows spring.config.activate.on-profile to be used in a profile-specific file. That makes that entire example invalid and we need to remove it. Sorry about the confusion. Since this change will go in 2.4.3, we would like to update this bit before it is released (Feb 18th). Will you be able to make the change that removes this section before that? No worries if you can't, we can take care of it.

@friscoMad
Copy link
Contributor Author

@friscoMad friscoMad commented Feb 17, 2021

Ok, after reviewing the mentioned ticket I think I understand my misunderstanding. I am working with 2.4.1 where that feature is working, it was on 2.4.2 when this stopped working, that's why I did not understand the example.
That said, this paragraph was there in the docs before 2.4, so it was not added with 2.4.2 regression, I don't know what was the behavior in older versions but I agree with you that if what you describe was the original intent of the documentation is better to just remove it.
I will modify it and remove the paragraph.

…e section as a warning.
@friscoMad
Copy link
Contributor Author

@friscoMad friscoMad commented Feb 17, 2021

@mbhave Done! Please confirm that this is what you expected.

@mbhave
Copy link
Contributor

@mbhave mbhave commented Feb 17, 2021

Thanks @friscoMad. This has been merged into 2.4.x and master. I decided to move the warning to the bottom of the section.

@wilkinsona wilkinsona changed the title Update yaml shortcomings wrong example Remove YAML shortcomings section as it no longer applies Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants