Skip to content
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

return context with secret material with getSecretAndContext #258

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kiran
Copy link

@kiran kiran commented Nov 1, 2019

When getting a secret, it's sometimes helpful to retrieve extra context associated with it.

Note: I'm not tied to the shape of the return value.

@kiran kiran changed the title return extra context with getSecret return context with secret material with getSecretAndContext Nov 1, 2019
@mike-luminal mike-luminal self-assigned this Jan 25, 2020
Copy link
Contributor

@mike-luminal mike-luminal left a 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, but can you rename this to getSecretAndMetadata to reduce the chance of confusion with the KMS Encryption Context?

@kiran
Copy link
Author

kiran commented Jan 30, 2020

@mike-luminal ah, good point. The PR is updated with the new naming.

@mike-luminal
Copy link
Contributor

Hmm, it looks like this would change the default output of credstash get <secret>. Could you preserve the existing behavior and add an option such as --with-metadata to credstash get for the full output with metadata?

@kiran
Copy link
Author

kiran commented Feb 12, 2020

Yup, sorry!

$ credstash put -a --comment="testing comments" testsecret fake
testsecret has been stored
$ credstash get testsecret
fake
$ credstash get -m testsecret
('fake', {'version': '0000000000000000002', 'comment': 'testing comments'})
$ credstash get --include-metadata testsecret
('fake', {'version': '0000000000000000002', 'comment': 'testing comments'})

@kiran kiran requested a review from mike-luminal March 31, 2020 19:07
@jason-fugue
Copy link
Contributor

Thanks for the PR! And sorry for the delay on this after you made the requested changes. This all looks good to me. Could you please resolve the merge conflicts and I'll get this merged in?

@kiran
Copy link
Author

kiran commented Feb 9, 2022

@jason-fugue thanks for the review!
I resolved the merge conflicts, and ran

$ credstash put -a --comment="testing comments" testsecret fake
testsecret has been stored
$ credstash get testsecret
fake
$ credstash get -m testsecret
('fake', {'version': '0000000000000000002', 'comment': 'testing comments'})
$ credstash get --include-metadata testsecret
('fake', {'version': '0000000000000000002', 'comment': 'testing comments'})

again, just to test, as well as rerunning the unit tests.

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.

3 participants