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

spring.config.activate.on-cloud-platform should consider NONE to be semantically equivalent to null #38506

Closed
osirisOfGit opened this issue Nov 22, 2023 · 3 comments
Labels
status: superseded An issue that has been superseded by another

Comments

@osirisOfGit
Copy link

osirisOfGit commented Nov 22, 2023

Apologies if this has been reported before, couldn't find any StackOverflow posts or GitHub issues.

When attempting to use spring.config.activate.on-cloud-platform, there is no mechanism available to simply state "If not running on a cloud platform" without forcibly setting the NONE value via spring.main.cloud-platform through some other mechanism.

return this.onCloudPlatform == null || this.onCloudPlatform == cloudPlatform;

This is undesirable for libraries providing configuration files with default values (e.g. through multi-documents) for scenarios when the consumers is running on a cloud platform and for when they aren't, as it forces one of the following scenarios:

  • Force consumers to specify spring.main.cloud-platform: NONE when running locally (ex: through their application-{local-env-name}.yml)
  • Abandon spring.config.activate.on-cloud-platform and multi-document config files, and conditionally load the properties through custom beans and manual CloudPlatform checks
  • Abandon spring.config.activate.on-cloud-platform and use a different, likely more unreliable, property condition, such as spring.config.activate.on-profile: 'DEV | LOCAL | NO-PLATFORM'

Proposed change:

		private boolean isActive(CloudPlatform cloudPlatform) {
			return this.onCloudPlatform == null 
                                       || (this.onCloudPlatform == NONE && cloudPlatform == null) 
                                       || this.onCloudPlatform == cloudPlatform);
		}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 22, 2023
quaff added a commit to quaff/spring-boot that referenced this issue Nov 23, 2023
@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Nov 29, 2023
@wilkinsona
Copy link
Member

We think this makes sense but we're unsure about when to make the change. It could be considered a bug, in which case 3.1.x would be an option if the risk isn't too great. WDYT, @philwebb?

@philwebb
Copy link
Member

philwebb commented Dec 6, 2023

I think we should consider this an enhancement.

The original intention of the spring.config.activate.on-cloud-platform property was to allow additional or override configuration for a specific cloud platform, I don't think we really considered none being used there.

The CloudPlatform.NONE enum was primary added to allow users to prevent our detection logic from running. From the javadoc:

No Cloud platform. Useful when false-positives are detected.

In other words, if we're guessing wrong for whatever reason then set this to tell us.

There is a subtle distinction between CloudPlatform.getActive(...) returning null and NONE. One means that we didn't detect a cloud platform, the other means the user actively stopped us from trying.

I think it's fine for us to change the isActive code in a minor release, since it feels like the change makes things more logical. I don't think we should do it in a patch release just in case someone is relying on the existing behavior.

@wilkinsona
Copy link
Member

Closing in favor of #38510 that we'll merge in 3.3.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Dec 7, 2023
@wilkinsona wilkinsona added status: superseded An issue that has been superseded by another and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
4 participants