Skip to content
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

Separate diff decision from presentation #2379

Merged
merged 86 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
86 commits
Select commit Hold shift + click to select a range
6a81da1
expand diff cross tests
VenelinMartinov Aug 26, 2024
f4de62c
verify order on replace
VenelinMartinov Aug 26, 2024
aafaaf8
add test cases
VenelinMartinov Aug 26, 2024
0b26e14
handle empty config
VenelinMartinov Aug 26, 2024
cc8af17
add tf action assertions
VenelinMartinov Aug 27, 2024
44cb678
detailed diff cross tests
VenelinMartinov Aug 27, 2024
ab4ec43
Merge branch 'master' into vvm/collection_only_detailed_diff_cross_test
VenelinMartinov Aug 30, 2024
8141c84
lint
VenelinMartinov Aug 30, 2024
e679a79
assert TF before and after not equal
VenelinMartinov Aug 30, 2024
ed194ba
separate diff decision from presentation
VenelinMartinov Aug 30, 2024
af59c07
lint
VenelinMartinov Aug 30, 2024
06a998c
revert change
VenelinMartinov Aug 30, 2024
8b2f4b6
add todos
VenelinMartinov Aug 30, 2024
159f75b
run all tests for PRC
VenelinMartinov Aug 30, 2024
94b0670
revert
VenelinMartinov Aug 30, 2024
59b963b
re-enable PRC for all, skip some tests
VenelinMartinov Aug 30, 2024
49134af
lint
VenelinMartinov Aug 30, 2024
222783e
skip more tests
VenelinMartinov Aug 30, 2024
ffe9d31
PRC test cleanup
VenelinMartinov Aug 30, 2024
32ea39f
Merge branch 'vvm/collection_only_detailed_diff_cross_test' into vvm/…
VenelinMartinov Aug 30, 2024
0fece77
revert some test changes
VenelinMartinov Aug 30, 2024
b9e97c0
revert
VenelinMartinov Aug 30, 2024
5d0f103
Merge branch 'vvm/PRC_cleanup' into vvm/separate_diff_decision
VenelinMartinov Aug 30, 2024
0208e84
skip panic test for now
VenelinMartinov Aug 30, 2024
06137ea
skip test
VenelinMartinov Aug 30, 2024
2e23036
fix test
VenelinMartinov Aug 30, 2024
8852cb5
revert more test changes
VenelinMartinov Aug 30, 2024
89b5f68
Merge branch 'vvm/PRC_cleanup' into vvm/separate_diff_decision
VenelinMartinov Aug 30, 2024
174d0c0
fix test
VenelinMartinov Aug 30, 2024
be0bb60
Merge branch 'vvm/PRC_cleanup' into vvm/separate_diff_decision
VenelinMartinov Aug 30, 2024
beec4df
remove test skip
VenelinMartinov Aug 30, 2024
133ae0a
accidental change
VenelinMartinov Aug 30, 2024
34a0bef
skip deceptive tests
VenelinMartinov Aug 30, 2024
b637050
re-record test
VenelinMartinov Sep 3, 2024
2ccf2fc
separate v1 and v2 tests
VenelinMartinov Sep 3, 2024
06a73a3
lint
VenelinMartinov Sep 3, 2024
ea5fd70
ignore set order in test
VenelinMartinov Sep 3, 2024
360deff
fix provider test panics
VenelinMartinov Sep 3, 2024
b8b22e3
add back PRC opt out
VenelinMartinov Sep 3, 2024
fac7d64
skip some more tests
VenelinMartinov Sep 3, 2024
7c39bb0
fix some more GRPC tests
VenelinMartinov Sep 3, 2024
fdb54f6
more GRPC test fixes
VenelinMartinov Sep 3, 2024
62bcc74
Merge branch 'vvm/PRC_cleanup' into vvm/separate_diff_decision
VenelinMartinov Sep 3, 2024
a922fa2
add todos for diff in wrong maxitemsone type case
VenelinMartinov Sep 3, 2024
ddb032f
fix other test
VenelinMartinov Sep 3, 2024
457544f
re-record tests
VenelinMartinov Sep 4, 2024
894bf44
skip detailed diff test which now fails
VenelinMartinov Sep 4, 2024
7c6b1cc
Revert "re-record tests"
VenelinMartinov Sep 4, 2024
7d1b457
revert detailed diff change
VenelinMartinov Sep 4, 2024
007b4a9
revert detailed diff test changes
VenelinMartinov Sep 4, 2024
a6e625c
revert more detailed diff changes
VenelinMartinov Sep 4, 2024
a1a0b97
detailed diff block tests
VenelinMartinov Sep 6, 2024
993b672
revert accidental changes
VenelinMartinov Sep 6, 2024
8d76df3
skip flaky test
VenelinMartinov Sep 6, 2024
3cf51f7
Merge branch 'vvm/detailed_diff_block_tests' into vvm/separate_diff_d…
VenelinMartinov Sep 6, 2024
a174362
Merge branch 'master' into vvm/collection_only_detailed_diff_cross_test
VenelinMartinov Sep 16, 2024
4eb2b84
Merge branch 'vvm/collection_only_detailed_diff_cross_test' into vvm/…
VenelinMartinov Sep 16, 2024
2085708
Merge branch 'vvm/PRC_cleanup' into vvm/separate_diff_decision
VenelinMartinov Sep 16, 2024
e224aac
Merge branch 'master' into vvm/collection_only_detailed_diff_cross_test
VenelinMartinov Sep 16, 2024
551a615
Merge branch 'vvm/collection_only_detailed_diff_cross_test' into vvm/…
VenelinMartinov Sep 16, 2024
47f9646
Merge branch 'vvm/PRC_cleanup' into vvm/separate_diff_decision
VenelinMartinov Sep 16, 2024
0fb7892
fix merge
VenelinMartinov Sep 16, 2024
5f5ceae
lint
VenelinMartinov Sep 16, 2024
d893052
fix test
VenelinMartinov Sep 16, 2024
66bcd25
remove panic tests for type checker
VenelinMartinov Sep 17, 2024
0c97572
fix state internals tests
VenelinMartinov Sep 17, 2024
0755a9c
Merge branch 'master' into vvm/PRC_cleanup
VenelinMartinov Sep 17, 2024
a6a081d
fix grpc test
VenelinMartinov Sep 17, 2024
41d30e5
Merge branch 'vvm/PRC_cleanup' into vvm/separate_diff_decision
VenelinMartinov Sep 17, 2024
7b49b80
remove todos
VenelinMartinov Sep 17, 2024
c23913a
remove redundant diffs
VenelinMartinov Sep 17, 2024
8983d73
add back deleted tests, add integration test
VenelinMartinov Sep 18, 2024
21098e8
correct test naming
VenelinMartinov Sep 18, 2024
023f2aa
correct expected output, add testing.T to subtests
VenelinMartinov Sep 18, 2024
0b70e85
Merge branch 'master' into vvm/PRC_cleanup
VenelinMartinov Sep 19, 2024
32f6385
allow disabling PRC
VenelinMartinov Sep 19, 2024
c6614da
add todos for unknown elements in sets
VenelinMartinov Sep 19, 2024
5da8c78
remove debug fails
VenelinMartinov Sep 19, 2024
ad004f3
Merge branch 'vvm/PRC_cleanup' into vvm/separate_diff_decision
VenelinMartinov Sep 19, 2024
b3d0975
add description of new hook
VenelinMartinov Sep 19, 2024
122c814
fix test to use sdkv2
VenelinMartinov Sep 19, 2024
6bf91fc
fix import
VenelinMartinov Sep 19, 2024
f8f7407
Merge branch 'master' into vvm/separate_diff_decision
VenelinMartinov Sep 20, 2024
8f79cec
add feature flag
VenelinMartinov Sep 20, 2024
474629f
address review
VenelinMartinov Sep 20, 2024
ab1262f
add comment for TF code
VenelinMartinov Sep 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions pkg/tfbridge/diff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2077,7 +2077,7 @@ func TestListNestedAddMaxItemsOne(t *testing.T) {
}

type diffTestCase struct {
resourceSchema map[string]*schema.Schema
resourceSchema map[string]*v2Schema.Schema
resourceFields map[string]*SchemaInfo
state resource.PropertyMap
inputs resource.PropertyMap
Expand All @@ -2088,11 +2088,11 @@ type diffTestCase struct {

func diffTest2(t *testing.T, tc diffTestCase) {
ctx := context.Background()
res := &schema.Resource{
res := &v2Schema.Resource{
Schema: tc.resourceSchema,
}
provider := shimv1.NewProvider(&schema.Provider{
ResourcesMap: map[string]*schema.Resource{
provider := shimv2.NewProvider(&v2Schema.Provider{
ResourcesMap: map[string]*v2Schema.Resource{
"p_resource": res,
},
})
Expand Down Expand Up @@ -2130,21 +2130,21 @@ func diffTest2(t *testing.T, tc diffTestCase) {
}

func TestChangingMaxItems1FilterProperty(t *testing.T) {
schema := map[string]*schema.Schema{
schema := map[string]*v2Schema.Schema{
"rule": {
Type: schema.TypeList,
Type: v2Schema.TypeList,
Required: true,
MaxItems: 1000,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
Elem: &v2Schema.Resource{
Schema: map[string]*v2Schema.Schema{
"filter": {
Type: schema.TypeList,
Type: v2Schema.TypeList,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
Elem: &v2Schema.Resource{
Schema: map[string]*v2Schema.Schema{
"prefix": {
Type: schema.TypeString,
Type: v2Schema.TypeString,
Optional: true,
},
},
Expand Down
3 changes: 3 additions & 0 deletions pkg/tfbridge/info/info.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@ type Provider struct {
// EnableZeroDefaultSchemaVersion makes the provider default
// to version 0 when no version is specified in the state of a resource.
EnableZeroDefaultSchemaVersion bool
// EnableAccurateBridgePreview makes the bridge use an experimental feature
// to generate more accurate diffs and previews for resources
EnableAccurateBridgePreview bool
VenelinMartinov marked this conversation as resolved.
Show resolved Hide resolved
}

// HclExampler represents a supplemental HCL example for a given resource or function.
Expand Down
44 changes: 35 additions & 9 deletions pkg/tfbridge/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ import (
)

type providerOptions struct {
defaultZeroSchemaVersion bool
defaultZeroSchemaVersion bool
enableAccurateBridgePreview bool
}

type providerOption func(providerOptions) (providerOptions, error)
Expand All @@ -68,6 +69,13 @@ func WithDefaultZeroSchemaVersion() providerOption { //nolint:revive
}
}

func withAccurateBridgePreview() providerOption {
return func(opts providerOptions) (providerOptions, error) {
opts.enableAccurateBridgePreview = true
return opts, nil
}
}

func getProviderOptions(opts []providerOption) (providerOptions, error) {
res := providerOptions{}
for _, o := range opts {
Expand Down Expand Up @@ -268,6 +276,11 @@ func newProvider(ctx context.Context, host *provider.HostClient,
if info.EnableZeroDefaultSchemaVersion {
opts = append(opts, WithDefaultZeroSchemaVersion())
}

if info.EnableAccurateBridgePreview || cmdutil.IsTruthy(os.Getenv("PULUMI_TF_BRIDGE_ACCURATE_BRIDGE_PREVIEW")) {
opts = append(opts, withAccurateBridgePreview())
}

p := &Provider{
host: host,
module: module,
Expand Down Expand Up @@ -1148,15 +1161,28 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
dd := makeDetailedDiffExtra(ctx, schema, fields, olds, news, diff)
detailedDiff, changes := dd.diffs, dd.changes

// There are some providers/situations which `makeDetailedDiff` distorts the expected changes, leading
// to changes being dropped by Pulumi.
// Until we fix `makeDetailedDiff`, it is safer to refer to the Terraform Diff attribute length for setting
// the DiffResponse.
// We will still use `detailedDiff` for diff display purposes.
if opts.enableAccurateBridgePreview {
if decision := diff.DiffEqualDecisionOverride(); decision != shim.DiffNoOverride {
if decision == shim.DiffOverrideNoUpdate {
changes = pulumirpc.DiffResponse_DIFF_NONE
} else {
changes = pulumirpc.DiffResponse_DIFF_SOME
}
}
} else {
// There are some providers/situations which `makeDetailedDiff` distorts the expected changes, leading
// to changes being dropped by Pulumi.
// Until we fix `makeDetailedDiff`, it is safer to refer to the Terraform Diff attribute length for setting
// the DiffResponse.
// We will still use `detailedDiff` for diff display purposes.

// See also https://github.com/pulumi/pulumi-terraform-bridge/issues/1501.
if !diff.HasNoChanges() {
changes = pulumirpc.DiffResponse_DIFF_SOME
}
}

// See also https://github.com/pulumi/pulumi-terraform-bridge/issues/1501.
if !diff.HasNoChanges() {
changes = pulumirpc.DiffResponse_DIFF_SOME
if changes == pulumirpc.DiffResponse_DIFF_SOME {
// Perhaps collectionDiffs can shed some light and locate the changes to the end-user.
for path, diff := range dd.collectionDiffs {
detailedDiff[path] = diff
Expand Down
4 changes: 4 additions & 0 deletions pkg/tfshim/sdk-v1/instance_diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ type v1InstanceDiff struct {
tf *terraform.InstanceDiff
}

func (d v1InstanceDiff) DiffEqualDecisionOverride() shim.DiffOverride {
return shim.DiffNoOverride
}

func (d v1InstanceDiff) applyTimeoutOptions(opts shim.TimeoutOptions) {
if opts.ResourceTimeout != nil {
err := d.encodeTimeouts(opts.ResourceTimeout)
Expand Down
4 changes: 4 additions & 0 deletions pkg/tfshim/sdk-v2/instance_diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ type v2InstanceDiff struct {
tf *terraform.InstanceDiff
}

func (d v2InstanceDiff) DiffEqualDecisionOverride() shim.DiffOverride {
return shim.DiffNoOverride
}

func (d v2InstanceDiff) applyTimeoutOptions(opts shim.TimeoutOptions) {
// This method is no longer used with PlanResourceChange; we handle timeouts more directly.
if opts.ResourceTimeout != nil {
Expand Down
38 changes: 31 additions & 7 deletions pkg/tfshim/sdk-v2/provider2.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,10 @@ func (s *v2InstanceState2) Meta() map[string]interface{} {
type v2InstanceDiff2 struct {
v2InstanceDiff

config cty.Value
plannedState cty.Value
plannedPrivate map[string]interface{}
config cty.Value
plannedState cty.Value
plannedPrivate map[string]interface{}
diffEqualDecisionOverride shim.DiffOverride
}

func (d *v2InstanceDiff2) String() string {
Expand Down Expand Up @@ -146,6 +147,10 @@ func (d *v2InstanceDiff2) ProposedState(
}, nil
}

func (d *v2InstanceDiff2) DiffEqualDecisionOverride() shim.DiffOverride {
return d.diffEqualDecisionOverride
}

// Provides PlanResourceChange handling for select resources.
type planResourceChangeImpl struct {
tf *schema.Provider
Expand Down Expand Up @@ -269,13 +274,31 @@ func (p *planResourceChangeImpl) Diff(
TfToken: t,
PlanState: plan.PlannedState,
})

//nolint:lll
// Taken from https://github.com/opentofu/opentofu/blob/864aa9d1d629090cfc4ddf9fdd344d34dee9793e/internal/tofu/node_resource_abstract_instance.go#L1024
// We need to unmark the values to make sure Equals works.
// Equals will return unknown if either value is unknown.
// START
unmarkedPrior, _ := st.UnmarkDeep()
VenelinMartinov marked this conversation as resolved.
Show resolved Hide resolved
unmarkedPlan, _ := plannedState.UnmarkDeep()
eqV := unmarkedPrior.Equals(unmarkedPlan)
eq := eqV.IsKnown() && eqV.True()
VenelinMartinov marked this conversation as resolved.
Show resolved Hide resolved
// END

diffOverride := shim.DiffOverrideUpdate
if eq {
diffOverride = shim.DiffOverrideNoUpdate
}

return &v2InstanceDiff2{
v2InstanceDiff: v2InstanceDiff{
tf: plan.PlannedDiff,
},
config: cfg,
plannedState: plannedState,
plannedPrivate: plan.PlannedPrivate,
config: cfg,
plannedState: plannedState,
diffEqualDecisionOverride: diffOverride,
plannedPrivate: plan.PlannedPrivate,
}, err
}

Expand Down Expand Up @@ -507,7 +530,8 @@ func (s *grpcServer) PlanResourceChange(
PlannedState cty.Value
PlannedPrivate map[string]interface{}
PlannedDiff *terraform.InstanceDiff
}, error) {
}, error,
) {
configVal, err := msgpack.Marshal(config, ty)
if err != nil {
return nil, err
Expand Down
13 changes: 13 additions & 0 deletions pkg/tfshim/shim.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,25 @@ type ResourceAttrDiff struct {
Type DiffAttrType
}

type DiffOverride string

const (
DiffNoOverride DiffOverride = "no-override"
DiffOverrideNoUpdate DiffOverride = "no-update"
DiffOverrideUpdate DiffOverride = "update"
)

type InstanceDiff interface {
Attribute(key string) *ResourceAttrDiff
HasNoChanges() bool
ProposedState(res Resource, priorState InstanceState) (InstanceState, error)
Destroy() bool
RequiresNew() bool

// DiffEqualDecisionOverride can return a non-null value to override the default decision of if the diff is equal.
//
// DiffEqualDecisionOverride is only respected when EnableAccurateBridgePreview is set.
DiffEqualDecisionOverride() DiffOverride
}

type ValueType int
Expand Down
12 changes: 8 additions & 4 deletions pkg/tfshim/tfplugin5/instance_diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ type instanceDiff struct {
attributes map[string]shim.ResourceAttrDiff
}

func (d instanceDiff) DiffEqualDecisionOverride() shim.DiffOverride {
return shim.DiffNoOverride
}

func (d instanceDiff) applyTimeoutOptions(opts shim.TimeoutOptions) {
if opts.ResourceTimeout != nil {
err := d.encodeTimeouts(opts.ResourceTimeout)
Expand All @@ -39,8 +43,8 @@ func (d instanceDiff) applyTimeoutOptions(opts shim.TimeoutOptions) {
}

func newInstanceDiff(config, prior, planned cty.Value, meta map[string]interface{},
requiresReplace []*proto.AttributePath) *instanceDiff {

requiresReplace []*proto.AttributePath,
) *instanceDiff {
attributes, requiresNew := computeDiff(prior, planned, requiresReplace)
return &instanceDiff{
config: config,
Expand Down Expand Up @@ -216,8 +220,8 @@ func rangeValue(val cty.Value, each func(k, v cty.Value)) {
}

func computeDiff(prior, planned cty.Value,
requiresReplace []*proto.AttributePath) (map[string]shim.ResourceAttrDiff, bool) {

requiresReplace []*proto.AttributePath,
) (map[string]shim.ResourceAttrDiff, bool) {
requiresNew := stringSet{}
for _, path := range requiresReplace {
requiresNew.add(pathString(path))
Expand Down