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

fix & refactor(cloud-config-server): Issue with non-existing file/folder location used in spring cloud config server properties while upgrading spring boot 2.4.x. #1002

Merged

Conversation

j-sandy
Copy link
Contributor

@j-sandy j-sandy commented Dec 13, 2022

With spring boot 2.4, spring cloud config server location properties will no longer fail silently if the file or folder does not exist. Now, requires prefix "optional:" for the location.

https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-2.4-Release-Notes#config-file-processing-application-properties-and-yaml-files

Added testcase in support of code change to introduce "optional:" prefix for location properties.

implementation project(':kork-secrets')
implementation "org.springframework.cloud:spring-cloud-context"
implementation "org.springframework.cloud:spring-cloud-config-server"
implementation "io.awspring.cloud:spring-cloud-aws-context:2.3.5"

testImplementation "org.springframework.boot:spring-boot-starter-test"
testImplementation "org.springframework.cloud:spring-cloud-starter-bootstrap"
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't all users of kork-cloud-config-server need to specify org.springframework.cloud:spring-cloud-starter-bootstrap as a dependency? We'd make their lives easier by making it a runtimeOnly dependency too, yes? And I bet here, testRuntimeOnly is sufficient since nothing needs this to compile, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update the dependency as testRuntimeOnly

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding runtimeOnly org.springframework.cloud:spring-cloud-starter-bootstrap ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

org.springframework.cloud:spring-cloud-starter-bootstrap is a marker dependency to enable the bootstrap of cloud config server (which is disabled by default) for spring-cloud-2020.0-Release. I agree with your concern that testRuntimeOnly will not transport this marker dependency to other spinnaker components for cloud config server startup and we should go for runtimeOnly.
However, I tried other option of introducing spring.cloud.bootstrap.enabled=true, as mentioned here, it is working successfully (tested on halyard by removing this) and seems to be better option. Please suggest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update the property.

@j-sandy j-sandy force-pushed the sb-upgrade-2-4-13-cloud-config-import branch 2 times, most recently from beff73d to 016f001 Compare December 14, 2022 17:56
@@ -6,6 +6,7 @@ dependencies {

implementation "commons-io:commons-io"
implementation "org.apache.commons:commons-lang3"
implementation project(':kork-config')
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried this locally and it looks like this only needs to be

Suggested change
implementation project(':kork-config')
testImplementation project(':kork-config')

so DefaultPropertiesBuilder is available to ConfigServerTestApplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the dependency as suggested.

…der location used in spring cloud config server properties while upgrading spring boot 2.4.x.

With spring boot 2.4, spring cloud config server location properties will no longer fail silently if the file or folder does not exist. Now, requires prefix "optional:" for the location.

https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-2.4-Release-Notes#config-file-processing-application-properties-and-yaml-files

Added testcase in support of code change to introduce "optional:" prefix for location properties.
@j-sandy j-sandy force-pushed the sb-upgrade-2-4-13-cloud-config-import branch from 016f001 to e094334 Compare December 15, 2022 06:08
@dbyron-sf dbyron-sf added the ready to merge Approved and ready for merge label Dec 15, 2022
@mergify mergify bot added the auto merged label Dec 15, 2022
@mergify mergify bot merged commit 08d3f2f into spinnaker:master Dec 15, 2022
dbyron-sf added a commit to dbyron-sf/halyard that referenced this pull request Dec 17, 2022
Now that spinnaker/kork#990 and
spinnaker/kork#1002 are in (and halyard consumes them), we don't
need these changes here anymore.

Revert "fix(web): adjust configuration so that the halyard daemon starts with spring boot 2.4 (spinnaker#1987)"

This reverts commit 4428105.
@j-sandy j-sandy deleted the sb-upgrade-2-4-13-cloud-config-import branch December 20, 2022 05:33
j-sandy pushed a commit to spinnaker/halyard that referenced this pull request Dec 20, 2022
Now that spinnaker/kork#990 and
spinnaker/kork#1002 are in (and halyard consumes them), we don't
need these changes here anymore.

Revert "fix(web): adjust configuration so that the halyard daemon starts with spring boot 2.4 (#1987)"

This reverts commit 4428105.
richard-timpson pushed a commit to richard-timpson/kork that referenced this pull request May 3, 2023
* fix(config): Issue with non-existing file/folder location used in Spring properties while upgrading spring boot 2.4.x. (spinnaker#990)

With spring boot 2.4, spring location properties will no longer fail silently if the file or folder does not exist. Now, requires prefix "optional:" for the location.

https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-2.4-Release-Notes#config-file-processing-application-properties-and-yaml-files

And introducing new spring additional data importing property suggested in the links below, to be used with Spring boot 2.4:

https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-Config-Data-Migration-Guide

https://docs.spring.io/spring-boot/docs/2.4.0/reference/html/spring-boot-features.html#boot-features-external-config-files-importing

Added testcase in support of code change to introduce "optional:" prefix for location properties.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

@W-12236865

* fix & refactor(cloud-config-server): Issue with non-existing file/folder location used in spring cloud config server properties while upgrading spring boot 2.4.x. (spinnaker#1002)

With spring boot 2.4, spring cloud config server location properties will no longer fail silently if the file or folder does not exist. Now, requires prefix "optional:" for the location.

https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-2.4-Release-Notes#config-file-processing-application-properties-and-yaml-files

Added testcase in support of code change to introduce "optional:" prefix for location properties.

@W-12236865

Co-authored-by: Sandesh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants