-
Notifications
You must be signed in to change notification settings - Fork 217
Spring Boot Cloud Config #1230
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
base: master
Are you sure you want to change the base?
Spring Boot Cloud Config #1230
Conversation
25b3e33
to
2b4022b
Compare
@lony2003 first of all, thanks so so much for contributing back with this. This is highly appreciated.. I will start adding comments in the PR with small details that we need to fix and questions about how this would work. It will be great if you can add as part of this PR an example on the |
...ain/java/io/dapr/spring/boot/cloudconfig/autoconfigure/DaprCloudConfigAutoConfiguration.java
Outdated
Show resolved
Hide resolved
import io.dapr.spring.boot.autoconfigure.client.DaprClientProperties; | ||
import io.dapr.spring.boot.autoconfigure.client.DaprConnectionDetails; | ||
|
||
class CloudConfigPropertiesDaprConnectionDetails implements DaprConnectionDetails { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lony2003 do we need to duplicate the DaprConnectionDetails, why are these different for the normal DaprConnectionDetails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No difference, just because the PropertyDaprConnectionDetails is protected and can't be used in another package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@artur-ciocanu should we change the PropertyDaprConnectionDetails
visibility so it can be reused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...onfig/src/main/java/io/dapr/spring/boot/cloudconfig/config/DaprCloudConfigClientManager.java
Outdated
Show resolved
Hide resolved
...udconfig/src/main/java/io/dapr/spring/boot/cloudconfig/config/DaprCloudConfigProperties.java
Show resolved
Hide resolved
Thanks. the code is not completed currently, I created this pull request is for the problems in issue #1225 , I think we need a schema for cloud config url, and have some other things to be discussed😊😊😊 |
Cloud Config Import SchemasSecret Store Componenturl structure
paramters
demomultiValued = false:store content(file secret store as example){
"dapr.spring.demo-config-secret.singlevalue": "testvalue",
"multivalue-properties": "dapr.spring.demo-config-secret.multivalue.v1=spring\ndapr.spring.demo-config-secret.multivalue.v2=dapr",
"multivalue-yaml": "dapr:\n spring:\n demo-config-secret:\n multivalue:\n v3: cloud"
} valid demo url
multiValued = true, nestedSeparator = ".":store content(file secret store as example){
"value1": {
"dapr": {
"spring": {
"demo-config-secret": {
"multivalue": {
"v4": "config"
}
}
}
}
}
} will be read as {
"value1": {
"dapr.spring.demo-config-secret.multivalue.v4": "config"
}
} valid demo url
Configuration Componenturl structure
paramters
Demostore content(table as example)
valid demo url
|
383a076
to
034baad
Compare
All jobs have done, waiting for review. Note that subscription of configuration api is not implemented, just as a reservation. |
...ng-cloudconfig/src/test/java/io/dapr/spring/boot/cloudconfig/CloudConfigTestApplication.java
Show resolved
Hide resolved
dapr-spring/dapr-spring-cloudconfig/src/test/resources/application.yaml
Outdated
Show resolved
Hide resolved
I have checked the error, and it seems that jacoco coverage throws the error. I have add a test of bulk secret to coverage more situation in |
@lony2003 this is amazing! thank so much for all the work that you put on this PR! |
Signed-off-by: lony2003 <[email protected]>
Signed-off-by: lony2003 <[email protected]>
…porter Implemented the configuration importer using dapr client configuration api also done a style check Signed-off-by: lony2003 <[email protected]>
Signed-off-by: lony2003 <[email protected]>
Signed-off-by: lony2003 <[email protected]>
Springboot‘s PropertySourceLoader will return a invalid source if it doesn't support that instead of throw errors. So we have to clear that what kind of loader we want to use, that is , that kind of resource it is. Signed-off-by: lony2003 <[email protected]>
Enum of Config Type has lack of file extension suport, reimplement it as class Signed-off-by: lony2003 <[email protected]>
… and config store Signed-off-by: lony2003 <[email protected]>
…fig store (try1) Signed-off-by: lony2003 <[email protected]>
…cloudconfig Signed-off-by: lony2003 <[email protected]>
Signed-off-by: lony2003 <[email protected]>
Signed-off-by: lony2003 <[email protected]>
add a test of Dapr Secret getBulkSecret method Signed-off-by: lony2003 <[email protected]>
Signed-off-by: lony2003 <[email protected]>
Signed-off-by: lony2003 <[email protected]>
Signed-off-by: lony2003 <[email protected]>
Fix the example doc to make cloud config demo works both local and kubernetes, and pass the mm doc validate Signed-off-by: lony2003 <[email protected]>
(cherry picked from commit 31a47f5) Signed-off-by: lony2003 <[email protected]>
Fix the example doc to make cloud config demo works both local and kubernetes Signed-off-by: lony2003 <[email protected]>
Signed-off-by: lony2003 <[email protected]>
2bed2a2
to
70d4eea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1230 +/- ##
============================================
- Coverage 76.91% 75.31% -1.60%
- Complexity 1592 1707 +115
============================================
Files 145 211 +66
Lines 4843 5473 +630
Branches 562 594 +32
============================================
+ Hits 3725 4122 +397
- Misses 821 1004 +183
- Partials 297 347 +50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@dapr/maintainers-java-sdk I believe that we can merge this PR, we will incrementally improve the quality of this PR, but I want this module to be present in the SDK. Please approve, we can fix any findings in smaller PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lony2003 thanks a lot for your contribution! I have reviewed the PR and I have left quite a few comments.
The main issue that I want to highlight is that we need to make sure that we can leverage ConfigurableBootstrapContext
and BootstrapRegistryInitializer
to ensure we create the DaprClient
and DaprClientProperties
beans. As far as I saw a lot of complexity is related to the fact that we try to jump through the hoops just to get the right instances so we could load the data.
While I have mentioned DaprClient
and DaprClientProperties
beans, all the other dependencies like parse handler should be registered with the ConfigurableBootstrapContext
as well, so we could look them up when needed.
Using the aforementioned approach will allow us to simplify the code and be able to expand in the future without a lot of effort.
@@ -16,8 +16,10 @@ | |||
import io.dapr.spring.data.DaprKeyValueAdapterResolver; | |||
import org.springframework.boot.context.properties.ConfigurationProperties; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer using a simple string prefix like "dapr.client"
<description>Dapr Spring Cloud Config</description> | ||
<packaging>jar</packaging> | ||
|
||
<dependencies> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell ONLY dapr-spring-boot-autoconfigure
dependency should be required all the others seems to be copy-pasted from other POM.
Could we please make sure that ONLY the required dependencies are used.
import io.dapr.spring.boot.autoconfigure.client.DaprClientProperties; | ||
import io.dapr.spring.boot.autoconfigure.client.DaprConnectionDetails; | ||
|
||
class CloudConfigPropertiesDaprConnectionDetails implements DaprConnectionDetails { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,9 @@ | |||
package io.dapr.springboot.examples.cloudconfig; | |||
|
|||
public class DaprConfigurationStores { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have this extracted to src/test/resources
@@ -0,0 +1,23 @@ | |||
package io.dapr.springboot.examples.cloudconfig; | |||
|
|||
public class DaprSecretStores { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract this to src/test/resources
" \"multivalue-properties\": \"dapr.spring.demo-config-secret.multivalue.v1=spring\\ndapr.spring.demo-config-secret.multivalue.v2=dapr\",\n" + | ||
" \"multivalue-yaml\": \"dapr:\\n spring:\\n demo-config-secret:\\n multivalue:\\n v3: cloud\"\n" + | ||
"}"; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract this to src/test/resources
|
||
@SpringBootApplication | ||
public class TestCloudConfigApplication { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to main()
method?
name: dapr.spring.demo-config-secret.singlevalue | ||
type: Opaque | ||
stringData: | ||
dapr.spring.demo-config-secret.singlevalue: "testvalue" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add new line
@lony2003 also could you please sync your branch with |
@artur-ciocanu I am working on my graduation currently, once I have time, I‘ll resolve them. @salaboy Thank you for approving this. |
@lony2003 as soon as you have the changes done please let me know |
@artur-ciocanu thank so much for all the reviews and unblocking |
Description
A new library that implement a backend of Spring Cloud Config.
Originally from https://github.com/fangkehou-team/dapr-spring, this library created a backend of SpringCloudConfig just like SpringCloudVault.
The original library only uses secret store as config store api is not stable at that time.
As the configuration api is stable now, the config loader using that api has been implemented, but subscription of configuration has not implemented, just for reservation.
Issue reference
this PR will close: #1225
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: