Skip to content

Conversation

markbigler
Copy link
Contributor

If I'm not wrong, currently nothing will be logged because the logging system has not yet been initialized when the EnvironmentPostProcessor's are invoked.

This PR makes use of the DeferredLog, so the log entries are no longer lost and become visible as soon as the logging system is ready.

Mark Bigler added 2 commits November 15, 2019 11:31
Use DeferredLog in CloudFoundryVcapEnvironmentPostProcessor since the logging system is not yet initialized when the EnvironmentPostProcessor's are invoked.
Receiving the ApplicationPreparedEvent is a prerequisite for being able to use DeferredLog in CloudFoundryVcapEnvironmentPostProcessor.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 15, 2019
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch and thank you for the PR!

I am not sure yet if we should backport this to 2.1.x or not, flagging for team attention. I've also added a minor comment. We can take care of it as part of the merge or you can update this PR if you have time.

@@ -89,9 +90,9 @@
* @author Andy Wilkinson
* @since 1.3.0
*/
public class CloudFoundryVcapEnvironmentPostProcessor implements EnvironmentPostProcessor, Ordered {
public class CloudFoundryVcapEnvironmentPostProcessor implements EnvironmentPostProcessor, Ordered, ApplicationListener<ApplicationEvent> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could implement ApplicationListener<ApplicationPreparedEvent> instead and avoid the if check in the listener implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing that out. Fixed with a9c1e03

@snicoll snicoll added for: team-attention An issue we'd like other members of the team to review type: bug A general bug labels Nov 16, 2019
Directly listen for ApplicationPreparedEvent instead of ApplicationEvent and avoid the event type check.
@wilkinsona
Copy link
Member

This seems to me like something that we should fix in 2.1.x. The fix doesn't look risky so I suspect I've missed something. What's making you unsure, @snicoll?

@snicoll
Copy link
Member

snicoll commented Nov 19, 2019

I can’t remember now. Thanks for confirming!

@snicoll snicoll removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Nov 19, 2019
@snicoll snicoll added this to the 2.1.11 milestone Nov 19, 2019
@snicoll snicoll self-assigned this Nov 20, 2019
@snicoll snicoll closed this in c2221b9 Nov 21, 2019
@snicoll
Copy link
Member

snicoll commented Nov 21, 2019

@markbigler thank you for making your first contribution to Spring Boot. This is merged in 2.1.x as well as 2.2.x and master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants