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

Remove Redis secrets backend #35

Merged
merged 4 commits into from
Nov 7, 2024
Merged

Conversation

tomasz-sadura
Copy link
Contributor

No description provided.

@@ -110,8 +110,6 @@ func parseSecretsLookupURN(ctx context.Context, logger *slog.Logger, urn string)
return newSecretManager(ctx, logger, u, newGCPSecretsManager)
case "az":
return newSecretManager(ctx, logger, u, newAzSecretsManager)
case "redis":
Copy link

Choose a reason for hiding this comment

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

Can we also get rid of the whole URN stuff here too? We don't need to stuff everything into a string in cloudv2 repo - we only do that in connect because it's a commandline arg.

Ideally these are just the implementations of aws, gcp and az secret managers that satisfy some interface in the connect repo and can be plugged in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It makes perfect sense. Wanted to reuse the secret provider creation mechanism (from URL), but this way looks a lot simpler, thanks!

Copy link

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

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

I have an optional thing - I don't want to block this work, but I would love a less sneaky API.

secrets/gcp.go Outdated
@@ -18,7 +18,7 @@ type gcpSecretsManager struct {
logger *slog.Logger
}

func newGCPSecretsManager(ctx context.Context, logger *slog.Logger, url *url.URL) (secretAPI, error) {
func NewGCPSecretsManager(ctx context.Context, logger *slog.Logger, url *url.URL) (SecretAPI, error) {
Copy link

Choose a reason for hiding this comment

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

I don't have context on this PR, but it seems weird we pass in a URL here but it's actually a projectID in hiding. Can we simplify the interface here? Then we can keep the URN business in the CLI for connect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified

Copy link

Choose a reason for hiding this comment

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

This is awesome and just what I was thinking - thank you!

@tomasz-sadura tomasz-sadura merged commit 9ddb03b into main Nov 7, 2024
2 checks passed
@tomasz-sadura tomasz-sadura deleted the remove-redis-secrets-impl branch November 7, 2024 18:06
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.

2 participants