fix(internal/provider): set all supported envbuilder options #38
Conversation
| for key, elem := range data.ExtraEnv.Elements() { | ||
| if _, ok := env[key]; ok { | ||
| // This is a warning because it's possible that the user wants to override | ||
| // a value set by the provider. |
There was a problem hiding this comment.
Tbh we could go either way here, provider overrides extra, extra overrides provider or error when both are set. I think this is a good approach as any 👍🏻
There was a problem hiding this comment.
Let's go with this approach for now and tweak as required. Erroring here seems a bit extreme and there may be legit use-cases that we just can't think of right now.
| func (data *CachedImageResourceModel) setComputedEnv(ctx context.Context) diag.Diagnostics { | ||
| env := make(map[string]string) | ||
|
|
||
| env["ENVBUILDER_CACHE_REPO"] = tfValueToString(data.CacheRepo) |
There was a problem hiding this comment.
Interesting edge-case, what if extra env overrides cache repo? We use data.CacheRepo directly elsewhere, but it may be different in computed env.
We should probably always prefer the values from computed env when running the cache probe?
There was a problem hiding this comment.
We should probably always prefer the values from computed env when running the cache probe?
I don't agree. The provider should use the arguments it is supplied. We add a warning diag if you override existing environment variables set, which should be enough of an indication.
Alternatively, I could add a check for this and make it a diag.Error?
There was a problem hiding this comment.
I'm hesitant to block it with an error, so I'm just going to not allow overriding these but warn if they are set.
| diag = append(diag, ds...) | ||
| data.Env, ds = basetypes.NewListValueFrom(ctx, types.StringType, sortedKeyValues(env)) | ||
| diag = append(diag, ds...) | ||
| return diag |
There was a problem hiding this comment.
Thanks for moving all of this into one place! ❤️
There was a problem hiding this comment.
It was driving me up the wall 🙈
Adds support for setting all envbuilder options supported by the provider.
Other options that the provider overrides must be set via
extra_env.