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
Add the ability to copy configs between stacks #4971
Conversation
9793932
to
2136bbd
Compare
2136bbd
to
259a203
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.
Some questions and suggestions. I also want to take this for a spin myself.
Tests?
c94dcce
to
28860fc
Compare
28860fc
to
c8c08e3
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.
Some more comments, but getting closer.
I'd also still love to see some tests. Perhaps add some for Map.Copy
in sdk/go/common/resource/config/map_test.go
(and Value.Copy
, if you add that, to sdk/go/common/resource/config/value_test.go
)?
For the tests, I could imagine writing a custom crypter amongst the tests like (untested):
// Crypter that just adds a prefix to the plaintext string when encrypting,
// and removes the prefix from the ciphertext when decrypting.
type prefixCrypter struct {
prefix string
}
func newPrefixCrypter(prefix string) Crypter {
return prefixCrypter{prefix: prefix}
}
func (c prefixCrypter) DecryptValue(ciphertext string) (string, error) {
return strings.TrimPrefix(ciphertext, c.prefix), nil
}
func (c prefixCrypter) EncryptValue(plaintext string) (string, error) {
return c.prefix + plaintext, nil
}
And then in tests, create a config map for the source stack with some secure values that use newPrefixCrypter("stackA")
, and then copy to a "new stack" with newPrefixCrypter("stackB")
and confirm the secure values in the result match as expected for "stackB".
Thanks for the review @justinvp - just working on the tests right now - this will be ready again for review soon |
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.
Two more comments, otherwise LGTM! 🎉
Make the suggested changes and will merge on a successful build |
Fixes: #1583
Please note, I specifically tried to keep this code simple. As part of the work to change secrets provider, we will need to refactor some of this code around the building of a list of config to encrypt with a new provider