-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
vault-secrets: enhances dynamic secret output #129
Conversation
displayer := newDisplayer().OpenAppSecrets(resp.Payload.Secret).SetDefaultFormat(format.Pretty). | ||
SetSingleSecret(resp.Payload.Secret.Type) |
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.
A boolean argument to newDisplayer()
was the indication of displaying a single secret or a list of secrets before. With this change, I've introduced a SetSingleSecret(type)
. I think this is clearer but could use feedback.
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.
When chaining OpenAppSecrets(resp.Payload.Secret)
it gives the displayer instance access to the secret's type.
If the displayer internally had a map[secretType]openSecretFormatter
populated in automatically in newDisplayer()
it'd encapsulate that complexity for the callers and remove the need to use switch cases.
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 like this idea. Reworking it now!
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.
Okay I introduced an interface and removed all of the secret type switch cases in 6cd82c7. I was also able to simplify by deleting SetSecretType
and the private single
method. Those details are encapsulated now so that callers don't need to set them.
I have no super strong opinion, but the latest version is useful information for kv/rotating secrets so I'd lean towards keeping it. What about something like:
Instead of a |
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.
Looks good, 2 small suggestions for the list response and to simplify/eliminate the SetSingleSecret
if we think it's worth it, but that can totally be done as follow-ups if ever.
@maxcoulombe - Good suggestion! Using |
Changes proposed in this PR:
This PR enhances the output for dynamic secrets. Additionally, it makes the
displayer
the single source for fields to better encapsulate them. Adding a "secret type" to the displayer was necessary to be able to conditionally display fields.List secrets:
Read dynamic secret:
Open secrets:
How I've tested this PR:
Ran
make go/test
and manual testing. I will address test coverage once I get feedback this is directionally okay.How I expect reviewers to test this PR:
Run commands above with different secret types to see if there are any bugs / missing information.
Checklist: