Skip to content

Conversation

@sbchaos
Copy link
Contributor

@sbchaos sbchaos commented Jan 25, 2022

No description provided.

arinda-arif and others added 2 commits January 25, 2022 13:30
* feat(optimus): add namespace name to RegisterSecretRequest

* feat(optimus): add option for update_only in RegisterSecretRequest

* feat(optimus): add updateSecret api

Co-authored-by: Sandeep Bhardwaj <[email protected]>
}

message ListSecretsResponse {
message Secret {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we extract this message at root level so we don't need to create again for Get call? We can add a value field as well but return it empty for List call? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of now we don't have a GET call, we can extract this message when we have the GET and add required fields.

Copy link
Member

Choose a reason for hiding this comment

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

That will require us to refractor the code again to use a different proto message right? Why not have it designed like that from the start?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not have any GET call planned to expose cleartext secrets over api.

Choose a reason for hiding this comment

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

Yes, get on a secret isn't planned, and the expectation from read is to return the plaintext secret, so the message will change again. So, we can keep it under SecretList

@sbchaos sbchaos merged commit da1e483 into main Feb 14, 2022
@sbchaos sbchaos deleted the secrets-list-api branch February 14, 2022 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants