Skip to content

feat: add secret push format to AWS secrets manager#3189

Merged
Skarlso merged 5 commits intoexternal-secrets:mainfrom
Skarlso:issue_1816_push_secret_type
Mar 10, 2024
Merged

feat: add secret push format to AWS secrets manager#3189
Skarlso merged 5 commits intoexternal-secrets:mainfrom
Skarlso:issue_1816_push_secret_type

Conversation

@Skarlso
Copy link
Copy Markdown
Contributor

@Skarlso Skarlso commented Feb 25, 2024

Problem Statement

What is the problem you're trying to solve?

Related Issue

Fixes #1816

Proposed Changes

How do you like to solve the issue and why?

Checklist

  • I have read the contribution guidelines
  • All commits are signed with git commit --signoff
  • My changes have reasonable test coverage
  • All tests pass with make test
  • I ensured my PR is ready for review with make reviewable

@Skarlso Skarlso marked this pull request as ready for review February 25, 2024 21:44
@Skarlso Skarlso requested a review from a team as a code owner February 25, 2024 21:44
@Skarlso Skarlso requested a review from moolen February 25, 2024 21:44
Copy link
Copy Markdown
Contributor

@shuheiktgw shuheiktgw left a comment

Choose a reason for hiding this comment

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

I left a few comments but overall LGTM 🙂

if err != nil {
if errors.Is(err, errKeyNotFound) {
return def, nil
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe you forgot to return an error here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The intention here is that if the key is not found we return the set default value. Arguably I could have named the variable better. 🤣 or at least add a comment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I see what you are trying to do here 👍

const (
SecretPushFormatKey = "secretPushFormat"
SecretPushFormatString = "string"
SecretPushFormatBinary = "binary"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add documentation on these metadata?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks. I forgot! 🤦

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added :)

Skarlso added 5 commits March 8, 2024 17:01
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Mar 8, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.5% Duplication on New Code

See analysis details on SonarCloud

@Skarlso Skarlso changed the title add secret push format to AWS secrets manager feat: add secret push format to AWS secrets manager Mar 9, 2024
@Skarlso Skarlso requested a review from shuheiktgw March 9, 2024 16:40
Copy link
Copy Markdown
Contributor

@shuheiktgw shuheiktgw left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Skarlso Skarlso merged commit 1d5177c into external-secrets:main Mar 10, 2024
Bude8 pushed a commit to Bude8/external-secrets that referenced this pull request Jun 13, 2024
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.

PushSecret to AWS Secrets Manager: stored as Base64, results in AWS GUI error

2 participants