Skip to content

Polish #46505

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

Closed
wants to merge 3 commits into from
Closed

Polish #46505

wants to merge 3 commits into from

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Jul 22, 2025

This PR polishes a bit.

Signed-off-by: Johnny Lim <[email protected]>
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 22, 2025
@@ -1173,7 +1173,7 @@ Beans used for property conversion are requested very early during the applicati
Typically, any dependency that you require may not be fully initialized at creation time.
====

TIP: You may want to rename your custom javadoc:org.springframework.core.convert.ConversionService[] if it is not required for configuration keys coercion and only rely on custom converters qualified with javadoc:org.springframework.boot.context.properties.ConfigurationPropertiesBinding[format=annotation].
TIP: You may want to rename your custom javadoc:org.springframework.core.convert.ConversionService[] if it is not required for configuration keys coercion and only relies on custom converters qualified with javadoc:org.springframework.boot.context.properties.ConfigurationPropertiesBinding[format=annotation].
Copy link
Member

Choose a reason for hiding this comment

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

I think "rely" is correct here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wilkinsona Thanks for the feedback!

I reverted it in 32c9bbe.

assertThat(context).doesNotHaveBean(ZipkinSpanExporter.class);
assertThat(context).doesNotHaveBean(BytesEncoder.class);
});
new ApplicationContextRunner().withConfiguration(AutoConfigurations.of(OpenTelemetryConfiguration.class))
Copy link
Member

@wilkinsona wilkinsona Jul 22, 2025

Choose a reason for hiding this comment

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

OpenTelemetryConfiguration isn't an @AutoConfiguration so this is fine as it was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wilkinsona I don't know exactly what it does behind the scenes, but I saw the following code in the same class, so I thought that a part of the auto-configuration also uses AutoConfigurations.of():

private final ApplicationContextRunner contextRunner = new ApplicationContextRunner()
.withConfiguration(AutoConfigurations.of(DefaultEncodingConfiguration.class, OpenTelemetryConfiguration.class));

There are more occurrences like this, for example, as follows:

private final ApplicationContextRunner contextRunner = new ApplicationContextRunner()
.withConfiguration(AutoConfigurations.of(PulsarConfiguration.class))
.withBean(PulsarClient.class, () -> mock(PulsarClient.class));

I also thought that "User" in the withUserConfiguration() doesn't sound right for a part of the auto-configuration.

Are you suggesting that the mixed uses are okay, or that they need to be used with the withUserConfiguration() if it's not an @AutoConfiguration?

Copy link
Member

Choose a reason for hiding this comment

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

AutoConfigurations.of does two things:

  1. It sorts all of the passed-in configurations based on their before and after attributes
  2. It orders the configurations so that they're processed before any user configurations

In this case, the configuration isn't an auto-configuration so there are no before and after attributes to consider and there's only of them so there's nothing to sort. There's also only one configuration class in the test so the order in which they're processed doesn't matter either.

We're pretty inconsistent about this across our tests. What was there before is fine and what you've proposed is also fine. Given this, I'd prefer to leave things as they were.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wilkinsona Thanks for the explanation!

I reverted it in 00e2d1b.

@snicoll snicoll added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 25, 2025
@snicoll snicoll added this to the 3.4.9 milestone Jul 25, 2025
@snicoll snicoll self-assigned this Jul 25, 2025
@snicoll snicoll mentioned this pull request Jul 25, 2025
snicoll pushed a commit that referenced this pull request Jul 25, 2025
See gh-46505

Signed-off-by: Johnny Lim <[email protected]>
@snicoll snicoll closed this in d95e73e Jul 25, 2025
@izeye izeye deleted the polish-20250722 branch July 25, 2025 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants