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.

Sign up for GitHub

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

Jump to bottom

Fixes and Improvements with Vault KV Client #325

Merged
merged 4 commits into from
Mar 15, 2022
Merged

Fixes and Improvements with Vault KV Client #325

merged 4 commits into from
Mar 15, 2022

Conversation

turkenh
Copy link
Member

@turkenh turkenh commented Mar 13, 2022

Description of your changes

This is a follow-up PR after #322 with a couple of fixes and improvements:

  • Fixes owner not being set with delete, preventing deletion.
  • Refactors Vault KV client by separating into two clients for V1 and V2 (as suggested by @negz previously here)
  • Other minor improvements.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Unit tests and as described in crossplane/crossplane#2940

@turkenh
Fix owner not being set for delete
80debfa 
Signed-off-by: Hasan Turken <turkenh@gmail.com>
@turkenh
Use string instead if interface for KVSecret data
6f9579f 
Signed-off-by: Hasan Turken <turkenh@gmail.com>
@turkenh
Separate Vault KV client for v1 and v2
754abc4 
Signed-off-by: Hasan Turken <turkenh@gmail.com>
@turkenh turkenh requested review from negz and muvaf March 13, 2022 11:17
negz
negz reviewed Mar 14, 2022
View reviewed changes
Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

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

Looks good - I like these improvements. Please feel free to have anyone merge when you feel ready.

apis/common/v1/connection_details.go Outdated Show resolved Hide resolved
pkg/connection/manager.go Show resolved Hide resolved
pkg/connection/store/vault/store.go Show resolved Hide resolved
@turkenh
Add comment for linter and use types.UID for uid parameter
79ea2fe 
Signed-off-by: Hasan Turken <turkenh@gmail.com>
@turkenh turkenh mentioned this pull request Mar 15, 2022
Merged
2 tasks
@muvaf muvaf merged commit 988c9ba into crossplane:master Mar 15, 2022
@turkenh turkenh deleted the ess-fixes branch March 15, 2022 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@negz negz negz approved these changes

@muvaf muvaf Awaiting requested review from muvaf

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@turkenh @negz @muvaf