Additional fields in env config#2145
Conversation
| @@ -1,10 +1,11 @@ | |||
| package envconfig_test | |||
| package envconfig | |||
There was a problem hiding this comment.
Needed to change this to have access to knownProfileKeys in order to have a test that ensures it's in-sync with tags in tomlClientConfigProfile. Another option I considered was doing the key lookup at runtime (via reflection); that would remove the need for this change. Let me which tradeoff you prefer.
There was a problem hiding this comment.
Simple is best and most maintainable.
There was a problem hiding this comment.
In SDKs, we like to write tests that users might, and therefore in this case if you change the package, it will be hard for us to catch when we accidentally make something internal. Not a big deal though, but allowing all tests to access privates is not ideal for how we like to demonstrate what is something the user can author (matters more in other parts of the Go SDK where integration tests can help us show what we haven't exposed).
There was a problem hiding this comment.
Agree with Chad on both points, should be fine, we definitely want TestKnownProfileKeysInSync to test we're keeping these 2 in sync
stephanos
force-pushed
the
envconfig-add-fields
branch
from
4b9df7a to
5f03201
Compare
| var conf tomlClientConfig | ||
| conf.fromClientConfig(c) | ||
|
|
||
| // Encode to TOML then decode to map for merging additional fields |
There was a problem hiding this comment.
There is an optimization opportunity here to only do the re-encoding if AdditionalProfileFields is set, but that would add more code and increase complexity. Let me know if you prefer the opposite tradeoff here.
There was a problem hiding this comment.
My opinion: I don't think we need that level of performance optimization for the CLI operations. I'd prefer to minimize complexity and maximize readability and maintainability.
There was a problem hiding this comment.
Not sure it is much more complexity to add a conditional around this raw stuff, but meh, not a big deal to decode and re-encode for no reason
| var conf tomlClientConfig | ||
| conf.fromClientConfig(c) | ||
|
|
||
| // Encode to TOML then decode to map for merging additional fields |
There was a problem hiding this comment.
Not sure it is much more complexity to add a conditional around this raw stuff, but meh, not a big deal to decode and re-encode for no reason
| if err := toml.NewEncoder(&buf).Encode(&conf); err != nil { | ||
| return nil, err | ||
| } | ||
| var rawConf map[string]any | ||
| if _, err := toml.Decode(buf.String(), &rawConf); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Could also have the alternative approach of changing var conf tomlClientConfig + conf.fromClientConfig(c) to func (*ClientConfigToTOMLOptions) toTomlableMap() map[string]any, but since you're mutating a couple of layers deep, it might end up being encode/decode instead of explicit map anyways, so this is fine too.
| @@ -1,10 +1,11 @@ | |||
| package envconfig_test | |||
| package envconfig | |||
There was a problem hiding this comment.
In SDKs, we like to write tests that users might, and therefore in this case if you change the package, it will be hard for us to catch when we accidentally make something internal. Not a big deal though, but allowing all tests to access privates is not ideal for how we like to demonstrate what is something the user can author (matters more in other parts of the Go SDK where integration tests can help us show what we haven't exposed).
4 participants
What was changed
Adds ability to encode/decode additional profile keys in env config on top of pre-defined ones.
Why?
The Temporal CLI recently added support for an OAuth profile keys in the env config. It had to duplicate code from the envconfig package to do that. If the envconfig package supports encoding/decoding additional fields, that code can be deleted.
Also see previous convo: temporalio/cli#892 (comment)
See temporalio/cli#901 for how this would be used.
Checklist
Closes
How was this tested: