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

Kms #6

Merged
merged 14 commits into from
Sep 2, 2022
Merged

Kms #6

merged 14 commits into from
Sep 2, 2022

Conversation

hagabor
Copy link
Contributor

@hagabor hagabor commented Aug 18, 2022

Added KMS Service with GenerateDataKey function only (with parameters only for KeyId and NumberOfBytes).

@CLAassistant
Copy link

CLAassistant commented Aug 18, 2022

CLA assistant check
All committers have signed the CLA.

@oleiade oleiade self-assigned this Aug 18, 2022
@oleiade oleiade added the enhancement New feature or request label Aug 18, 2022
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

Thank you so much for your contribution @hagabor 🎉 🙇🏻 Much appreciated!

This looks excellent indeed. I've left a bunch of suggestions that are mainly targeted towards improving type-safety, naming, documentation and hiding AWS internal naming conventions. Please let me know if you see potential further refinements, and incorporate them 🙇🏻

I will be testing this against our AWS setup and get back to you as soon as I have confirmed for myself that it works as expected.

Note that I'll be out of office for the upcoming week, and might be slower to repond in that time period 😉

PS: Once we're good to go, I'll amend your PR with a regeneration of .map file as some type names will have changed 👍🏻

Cheers!

src/internal/kms.ts Show resolved Hide resolved
src/internal/kms.ts Outdated Show resolved Hide resolved
src/internal/kms.ts Outdated Show resolved Hide resolved
src/internal/kms.ts Outdated Show resolved Hide resolved
src/internal/kms.ts Outdated Show resolved Hide resolved
src/internal/kms.ts Outdated Show resolved Hide resolved
src/internal/kms.ts Outdated Show resolved Hide resolved
@oleiade
Copy link
Member

oleiade commented Aug 19, 2022

PR Checklist:

@oleiade oleiade added this to the 0.5.0 milestone Aug 30, 2022
@oleiade
Copy link
Member

oleiade commented Aug 30, 2022

I've tested the KMS feature you introduced @hagabor and everything seems to work as expected 🎉 Great job 🤝

Please go ahead and integrate the suggested changes, and we should be good to merge this in preparation for a v0.5.0 release. In order to stay productive, if I do not get an answer from you in the upcoming 7 days, I will go ahead and commit my suggestions and merge this so we don't stay blocked.

Cheers!

src/internal/kms.ts Outdated Show resolved Hide resolved
@hagabor
Copy link
Contributor Author

hagabor commented Aug 30, 2022

Dear Théo,

Sorry to responded with such delay. I'm going to integrate the suggested changes.

brgds,
Gábor

hagabor and others added 8 commits August 30, 2022 18:44
Co-authored-by: Théo Crevon <oleiade@users.noreply.github.com>
Co-authored-by: Théo Crevon <oleiade@users.noreply.github.com>
Co-authored-by: Théo Crevon <oleiade@users.noreply.github.com>
Co-authored-by: Théo Crevon <oleiade@users.noreply.github.com>
Co-authored-by: Théo Crevon <oleiade@users.noreply.github.com>
Co-authored-by: Théo Crevon <oleiade@users.noreply.github.com>
Co-authored-by: Théo Crevon <oleiade@users.noreply.github.com>
src/kms.ts Outdated Show resolved Hide resolved
src/kms.ts Outdated Show resolved Hide resolved
@hagabor
Copy link
Contributor Author

hagabor commented Aug 30, 2022

Dear Théo,

I've added some changes. One place I've left comment from the initial SecretsManager service (what I've used as starting point). Also changed export, and import symbols. Now seems working. Please review if I can ask you. Thanks!

Brgds,
Gábor

@oleiade
Copy link
Member

oleiade commented Sep 2, 2022

Hey @hagabor

Thanks a lot for this, much appreciated 🙇🏻
I'll merge this, and add end 2 end tests for it myself 👍🏻

@oleiade oleiade merged commit e35709d into grafana:main Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants