This repository has been archived by the owner on Jan 19, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add initial secret manager property source implementation #2168
Add initial secret manager property source implementation #2168
Changes from all commits
9c6bbd6
d713b8c
afe5e3f
eedb338
9781956
cfee77c
0afdf65
114eb1a
45d9f2b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Non-blocking, but a potential future optimization would be to parallelize this.
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.
Noted, added to #2176 to track.
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 think people will want to access specific secret versions. Defaulting to "latest" is not a production best practice.
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.
Ah good point, we'll definitely need an answer for this. I think we'll have to discuss a bit more in-depth with our team what is the best way to let users specify which secret versions they want to load into the environment; I guess we wouldn't want to load all versions of all secrets by default.
I added this point to #2176 to track; we will definitely implement this before releasing to users. I just want this PR to setup the initial scaffolding.
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 wonder if it makes sense to put this class into a new
spring-cloud-gcp-secretmanager
module. Then we could give people a single-dependency starter instead of having them bring in the autoconfiguration module combined with explicitly setting a property.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.
Shouldn't it just be conditional on class of the secret manager api rather than setting a property?
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.
+1
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.
Sounds good, we can discuss moving this in the next PR with the addition of secret template which will necessitate creating a the new secretmanager module. For now I am keeping the door open if we decide not to add a template class then this will mirror how our spring config support is organized (in which all the relevant classes are kept in autoconfigure/config).
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 add a tracking issue to add a template?
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.
Yes, this is tracked in #2176