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
Adds StackChangeSecretsProvider to Go automation api #14039
Conversation
Changelog[uncommitted] (2023-11-21)Features
|
4335f61
to
a6ecf6e
Compare
sdk/go/auto/local_workspace.go
Outdated
@@ -460,6 +461,19 @@ func (l *LocalWorkspace) Stack(ctx context.Context) (*StackSummary, error) { | |||
return nil, nil | |||
} | |||
|
|||
// StackChangeSecretsProvider edits the secrets provider for the given stack. | |||
func (l *LocalWorkspace) StackChangeSecretsProvider( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Rather than prefixing with Stack
, I am wondering if this should be ChangeStackSecretsProvider
. We have other stack operations around tags and import/export that aren't prefixed with Stack
.
Also, wondering if we should expose a ChangeSecretsProvider
on stack as well as a convenience.
sdk/go/auto/local_workspace.go
Outdated
@@ -460,6 +461,19 @@ func (l *LocalWorkspace) Stack(ctx context.Context) (*StackSummary, error) { | |||
return nil, nil | |||
} | |||
|
|||
// StackChangeSecretsProvider edits the secrets provider for the given stack. | |||
func (l *LocalWorkspace) StackChangeSecretsProvider( | |||
ctx context.Context, stackName, newSecretsProvider, stdin string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stdin string
I don't love calling this stdin
. If eventually we move away from a CLI-backed implementation under the covers, I'm not sure what stdin
even means anymore.
Also, what happens for the secret providers that don't need a value supplied to stdin? I assume I just pass an empty string in that case, but it's not immediately obvious from the parameter name that it's optional in that way.
I'm wondering if instead we should have something like:
type ChangeStackSecretsProviderOptions struct {
// NewPassphrase is the new passphrase when changing to a `passphrase` provider
NewPassphrase *string
}
ChangeStackSecretsProvider(ctx context.Context, stackName, newSecretsProvider string,
opts *ChangeStackSecretsProviderOptions) error
Where nil can be passed for the last parameter if not opts are needed.
Usage:
s.Workspace().StackChangeSecretsProvider(ctx, s.Name(), "default", nil)
newPassphrase := "newpassphrase"
s.Workspace().StackChangeSecretsProvider(ctx, s.Name(), "passphrase", &auto.ChangeStackSecretsProviderOptions{
NewPassphrase: &newPassphrase,
})
I could also imagine some alternative variants:
// 1. Single method with opts
ChangeStackSecretsProvider(ctx context.Context, stackName, newSecretsProvider string,
opts *ChangeStackSecretsProviderOptions) error
// 2. Single method with newPassphrase
ChangeStackSecretsProvider(ctx context.Context, stackName, newSecretsProvider string,
newPassphrase *string) error
// 3. Multiple methods, one without opts, one with opts
ChangeStackSecretsProvider(ctx context.Context, stackName, newSecretsProvider string) error
ChangeStackSecretsProviderWithOptions(ctx context.Context, stackName, newSecretsProvider string,
opts *ChangeStackSecretsProviderOptions) error
// 4. Single method with opts based on functional args pattern
ChangeStackSecretsProvider(ctx context.Context, stackName, newSecretsProvider string,
opts ...optchangesecretsprovider.Option) error
1 or 2 would be my preference.
Yes, but that will just be a passthrough to this method like most of the other Stack methods.
Likewise. This gets even worse if we start supporting more secret providers which might need even more complicated setup than just a single new passphrase.
I think this is my preference. It makes it possible for us to extend this with more options later if needed. |
Let's go with the naming and signature for (1) and also add a |
GitHub UI doesn't seem to be catching up with the commits on the branch. I'll give it a bit but we might need to close and re-create this PR. |
a6ecf6e
to
0515414
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Fixes #14035.
Checklist
make tidy
to update any new dependenciesmake lint
to verify my code passes the lint checkgofumpt
make changelog
and committed thechangelog/pending/<file>
documenting my change