-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Pluggable component: Secret store #6733
Pluggable component: Secret store #6733
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6733 +/- ##
==========================================
- Coverage 65.05% 64.90% -0.15%
==========================================
Files 231 232 +1
Lines 20595 20640 +45
==========================================
- Hits 13398 13397 -1
- Misses 6089 6126 +37
- Partials 1108 1117 +9
|
Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>
Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>
339dc99
to
7748354
Compare
d5bc87e
to
1356a7a
Compare
// GetSecretRequest is the message to get secret from secret store. | ||
message GetSecretRequest { | ||
// The name of secret key. | ||
string key = 2; |
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.
Silly question, but should field numbering generally start at 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.
it's not strictly necessary. The most important thing is that once you set a number, you never change it. https://protobuf.dev/programming-guides/proto3/#updating
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.
Yeah, I numbered it with 2 purposefully while trying to match the runtime/proto definition. I think, to make it consistent I will change with 1.
Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>
@artursouza @yaron2 @mukundansundar Can we merge this if it is being planned for 1.12 ? In that way I will try to get the sandbox-sdk PR also ready before 14th. |
Description
Issue reference
Please reference the issue this PR will close: #5635
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: