Implement DiffConfig #1683
Implement DiffConfig #1683
Conversation
| return plugin.DiffResult{}, fmt.Errorf("Error applying ignoreChanges: %v", err) | ||
| } | ||
|
|
||
| objDiff := oldInputs.DiffIncludeUnknowns(newInputsIC) |
There was a problem hiding this comment.
Is this appropriate to use here? Does the comment at L624 make sense? It sounds like this function almost supported ignoreChanges but not quite.
There was a problem hiding this comment.
It looks like a reasonable usage to me.
| @@ -21,7 +21,7 @@ import ( | |||
| "github.com/pulumi/pulumi/sdk/v3/go/common/resource" | |||
| ) | |||
|
|
|||
| func applyIgnoreChanges(old, new resource.PropertyMap, ignoreChanges []string) (resource.PropertyMap, error) { | |||
| func ApplyIgnoreChanges(old, new resource.PropertyMap, ignoreChanges []string) (resource.PropertyMap, error) { | |||
There was a problem hiding this comment.
Moving this around, kind of wondering are we sure there's no primitives in pulumi/pulumi that compute this or something like this? I found resource.PropertyValue has Contains now which interpreters matching against a pattern.
t0yv0
force-pushed
the
t0yv0/diffconfig
branch
from
8b89ed4
to
83fee4a
Compare
|
@Frassle reusing some of the code from p/p does this look reasonable. Also are there reference implementations for DiffConfig, sounds like it's a Pulumi-specific feature and the only "bridge"-related thing here is the reinterpretation of TF ForceNew as something indicating a replace plan in DiffConfig. Thanks. |
pkg/tfbridge/info.go
Outdated
| // also the section on Explicit Provider Configuration in the docs: | ||
| // | ||
| // https://www.pulumi.com/docs/concepts/resources/providers/ | ||
| EnableDiffConfig bool |
There was a problem hiding this comment.
I'm assuming that the arc of this variable will be:
EnableDiffConfigdefaults tofalse. We rollEnableDiffConfigout to a select set of our providers for live testing.EnableDiffConfigdefaults tofalse, but we have rolled it out individually to all of our tier 1 providers.EnableDiffConfigdefaults totrueand is enabled on all of our providers. We leave the option to disable to avoid blocking 3rd party providers if their provider doesn't work withEnableDiffConfig: true. We ask 3rd party providers who setEnableDiffConfig: falseto file a bug explaining what doesn't work.EnableDiffConfigis deprecated. The message says it will be removed in a future release.EnableDiffConfigis no longer an option. All bridged providers correctly implementDiffConfig.
To do that, we need to distinguish between true, false and unset:
| EnableDiffConfig bool | |
| EnableDiffConfig *bool |
| if resp := dominatingDiffResponse(responses); resp != nil { | ||
| return resp, nil | ||
| } |
There was a problem hiding this comment.
The logic below to merge diff responses should handle a single response correctly (merge({diff}) = diff). If it doesn't, that indicates a bug in merge.
What happens when we remove dominatingDiffResponse?
There was a problem hiding this comment.
I think this PR makes DiffConfig return ErrUnimplemented instead of the previous empty responses and it seems like these get returned from the muxer on line 282
There was a problem hiding this comment.
Yeah at first I observed it was swallowing detailedDiff seemingly intentionally. This is because it was merging a real diff with an empty diff from PF and selecting the PF one incorrectly. I since switched PF to return NotImplemented instead of a dummy empty response. I think this is how it should have been. With this change to NotImplemented in place I think the muxer needs to treat NotImplemented as a special error. dominatingDiffResponse is one way to do it but not the only way.
pkg/tfbridge/provider.go
Outdated
| schemaPath := PropertyPathToSchemaPath(path, p.schemaMap, p.schemaInfos) | ||
| schema, info, err := LookupSchemas(schemaPath, p.schemaMap, p.schemaInfos) |
There was a problem hiding this comment.
I love how easy PropertyPathToSchemaPath and LookupSchemas make this!
As part of #2148, this PR removes the duplicate implementation of `pulumigrpc.ResourceProviderServer` copied over from pulumi/pulumi and then modified. This is a post #2258 version of #2195, and is much less invasive. Addressing previous edits in response to #2195 (comment): - #1716: This change accommodated for a previous edit (`ProviderWithContext`) and is no longer necessary. That said, the tests it introduced are still present (and continue to pass), so we are sure this commit did not break the functionality. - #1683: Simply brings DiffConfig into line with pu/pu, which we get for free by not vendoring. - #1047: This is still handled in `provider_server.go`, and is still under test. - #1065: This has already been moved out of this area in #2258, so it is no longer relevant here.
As part of #2148, this PR removes the duplicate implementation of `pulumigrpc.ResourceProviderServer` copied over from pulumi/pulumi and then modified. This is a post #2258 version of #2195, and is much less invasive. Addressing previous edits in response to #2195 (comment): - #1716: This change accommodated for a previous edit (`ProviderWithContext`) and is no longer necessary. That said, the tests it introduced are still present (and continue to pass), so we are sure this commit did not break the functionality. - #1683: Simply brings DiffConfig into line with pu/pu, which we get for free by not vendoring. - #1047: This is still handled in `provider_server.go`, and is still under test. - #1065: This has already been moved out of this area in #2258, so it is no longer relevant here.
Fixes #244
Upon re-investigation DiffConfig appears to be a Pulumi-only feature as the notion of provider replacements, and especially cascading replacement plan from explicit provider to every resource managed by that provider, seems to be a Pulumi-only concept. The implementation is done accordingly at Pulumi-only level, not turning around through any TF internals. Path translation helpers are used to join in ForcesProviderReplace information from SchemaInfo overrides to changing property paths, and indicate replacement when a change is happening to such a property. Pulumi-level ignoreChanges is lifted from the Plugin Framework implementation and reused here.
Outside of the checked in tests, I have verified that the scenario outlined by @EvanBoyle in #244 (comment) is now made to work with AWS with this change. Getting there required taking some fixes to the muxer around DiffConfig muxing.