From d212a72d1da30f039f44185980291dca67a55e5b Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Mon, 13 Feb 2023 15:40:17 +0100 Subject: [PATCH] structured run output: impose canonical ordering on jsonstate and jsonplan packages (#32649) --- internal/addrs/resource.go | 69 +++++++---- internal/command/jsonformat/diff.go | 28 ----- internal/command/jsonformat/state.go | 23 +--- internal/command/jsonplan/plan.go | 15 ++- internal/command/jsonstate/state.go | 37 ++++-- .../provider-aliasing-conflict/output.json | 42 +++---- .../provider-aliasing-default/output.json | 86 ++++++------- .../show-json/provider-aliasing/output.json | 116 +++++++++--------- 8 files changed, 211 insertions(+), 205 deletions(-) diff --git a/internal/addrs/resource.go b/internal/addrs/resource.go index f400bcb421..bf19b2d247 100644 --- a/internal/addrs/resource.go +++ b/internal/addrs/resource.go @@ -32,6 +32,22 @@ func (r Resource) Equal(o Resource) bool { return r.Mode == o.Mode && r.Name == o.Name && r.Type == o.Type } +func (r Resource) Less(o Resource) bool { + switch { + case r.Mode != o.Mode: + return r.Mode == DataResourceMode + + case r.Type != o.Type: + return r.Type < o.Type + + case r.Name != o.Name: + return r.Name < o.Name + + default: + return false + } +} + func (r Resource) UniqueKey() UniqueKey { return r // A Resource is its own UniqueKey } @@ -100,6 +116,18 @@ func (r ResourceInstance) Equal(o ResourceInstance) bool { return r.Key == o.Key && r.Resource.Equal(o.Resource) } +func (r ResourceInstance) Less(o ResourceInstance) bool { + if !r.Resource.Equal(o.Resource) { + return r.Resource.Less(o.Resource) + } + + if r.Key != o.Key { + return InstanceKeyLess(r.Key, o.Key) + } + + return false +} + func (r ResourceInstance) UniqueKey() UniqueKey { return r // A ResourceInstance is its own UniqueKey } @@ -195,6 +223,18 @@ func (r AbsResource) Equal(o AbsResource) bool { return r.Module.Equal(o.Module) && r.Resource.Equal(o.Resource) } +func (r AbsResource) Less(o AbsResource) bool { + if !r.Module.Equal(o.Module) { + return r.Module.Less(o.Module) + } + + if !r.Resource.Equal(o.Resource) { + return r.Resource.Less(o.Resource) + } + + return false +} + func (r AbsResource) absMoveableSigil() { // AbsResource is moveable } @@ -308,30 +348,15 @@ func (r AbsResourceInstance) Equal(o AbsResourceInstance) bool { // Less returns true if the receiver should sort before the given other value // in a sorted list of addresses. func (r AbsResourceInstance) Less(o AbsResourceInstance) bool { - switch { - - case len(r.Module) != len(o.Module): - return len(r.Module) < len(o.Module) - - case r.Module.String() != o.Module.String(): + if !r.Module.Equal(o.Module) { return r.Module.Less(o.Module) - - case r.Resource.Resource.Mode != o.Resource.Resource.Mode: - return r.Resource.Resource.Mode == DataResourceMode - - case r.Resource.Resource.Type != o.Resource.Resource.Type: - return r.Resource.Resource.Type < o.Resource.Resource.Type - - case r.Resource.Resource.Name != o.Resource.Resource.Name: - return r.Resource.Resource.Name < o.Resource.Resource.Name - - case r.Resource.Key != o.Resource.Key: - return InstanceKeyLess(r.Resource.Key, o.Resource.Key) - - default: - return false - } + + if !r.Resource.Equal(o.Resource) { + return r.Resource.Less(o.Resource) + } + + return false } // AbsResourceInstance is a Checkable diff --git a/internal/command/jsonformat/diff.go b/internal/command/jsonformat/diff.go index 28d1dc6df9..9ddd50abf3 100644 --- a/internal/command/jsonformat/diff.go +++ b/internal/command/jsonformat/diff.go @@ -1,13 +1,10 @@ package jsonformat import ( - "sort" - "github.com/hashicorp/terraform/internal/command/jsonformat/computed" "github.com/hashicorp/terraform/internal/command/jsonformat/differ" "github.com/hashicorp/terraform/internal/command/jsonformat/differ/attribute_path" "github.com/hashicorp/terraform/internal/command/jsonplan" - "github.com/hashicorp/terraform/internal/command/jsonstate" "github.com/hashicorp/terraform/internal/plans" ) @@ -63,31 +60,6 @@ func precomputeDiffs(plan Plan, mode plans.Mode) diffs { diffs.outputs[key] = differ.FromJsonChange(output, attribute_path.AlwaysMatcher()).ComputeDiffForOutput() } - less := func(drs []diff) func(i, j int) bool { - return func(i, j int) bool { - left := drs[i].change - right := drs[j].change - - if left.ModuleAddress != right.ModuleAddress { - return left.ModuleAddress < right.ModuleAddress - } - - if left.Mode != right.Mode { - return left.Mode == jsonstate.DataResourceMode - } - - if left.Address != right.Address { - return left.Address < right.Address - } - - // Everything else being equal, we'll sort by deposed. - return left.Deposed < right.Deposed - } - } - - sort.Slice(diffs.drift, less(diffs.drift)) - sort.Slice(diffs.changes, less(diffs.changes)) - return diffs } diff --git a/internal/command/jsonformat/state.go b/internal/command/jsonformat/state.go index e200361f7e..0c181bca51 100644 --- a/internal/command/jsonformat/state.go +++ b/internal/command/jsonformat/state.go @@ -36,30 +36,11 @@ func (state State) GetSchema(resource jsonstate.Resource) *jsonprovider.Schema { } func (state State) renderHumanStateModule(renderer Renderer, module jsonstate.Module, opts computed.RenderHumanOpts, first bool) { - // Sort the resources in the module first, for consistent output. - var resources []jsonstate.Resource - resources = append(resources, module.Resources...) - sort.Slice(resources, func(i, j int) bool { - left := resources[i] - right := resources[j] - - if left.Mode != right.Mode { - return left.Mode == jsonstate.DataResourceMode - } - - if left.Address != right.Address { - return left.Address < right.Address - } - - // Everything else being equal, we'll sort by deposed. - return left.DeposedKey < right.DeposedKey - }) - - if len(resources) > 0 && !first { + if len(module.Resources) > 0 && !first { renderer.Streams.Println() } - for _, resource := range resources { + for _, resource := range module.Resources { if !first { renderer.Streams.Println() diff --git a/internal/command/jsonplan/plan.go b/internal/command/jsonplan/plan.go index 3c4960b410..fca01a1d28 100644 --- a/internal/command/jsonplan/plan.go +++ b/internal/command/jsonplan/plan.go @@ -329,7 +329,16 @@ func (p *plan) marshalPlanVariables(vars map[string]plans.DynamicValue, decls ma func MarshalResourceChanges(resources []*plans.ResourceInstanceChangeSrc, schemas *terraform.Schemas) ([]ResourceChange, error) { var ret []ResourceChange - for _, rc := range resources { + var sortedResources []*plans.ResourceInstanceChangeSrc + sortedResources = append(sortedResources, resources...) + sort.Slice(sortedResources, func(i, j int) bool { + if !sortedResources[i].Addr.Equal(sortedResources[j].Addr) { + return sortedResources[i].Addr.Less(sortedResources[j].Addr) + } + return sortedResources[i].DeposedKey < sortedResources[j].DeposedKey + }) + + for _, rc := range sortedResources { var r ResourceChange addr := rc.Addr r.Address = addr.String() @@ -491,10 +500,6 @@ func MarshalResourceChanges(resources []*plans.ResourceInstanceChangeSrc, schema } - sort.Slice(ret, func(i, j int) bool { - return ret[i].Address < ret[j].Address - }) - return ret, nil } diff --git a/internal/command/jsonstate/state.go b/internal/command/jsonstate/state.go index 0391d837a9..4bb2238073 100644 --- a/internal/command/jsonstate/state.go +++ b/internal/command/jsonstate/state.go @@ -323,8 +323,27 @@ func marshalModules( func marshalResources(resources map[string]*states.Resource, module addrs.ModuleInstance, schemas *terraform.Schemas) ([]Resource, error) { var ret []Resource + var sortedResources []*states.Resource for _, r := range resources { - for k, ri := range r.Instances { + sortedResources = append(sortedResources, r) + } + sort.Slice(sortedResources, func(i, j int) bool { + return sortedResources[i].Addr.Less(sortedResources[j].Addr) + }) + + for _, r := range sortedResources { + + var sortedKeys []addrs.InstanceKey + for k := range r.Instances { + sortedKeys = append(sortedKeys, k) + } + sort.Slice(sortedKeys, func(i, j int) bool { + return addrs.InstanceKeyLess(sortedKeys[i], sortedKeys[j]) + }) + + for _, k := range sortedKeys { + ri := r.Instances[k] + var err error resAddr := r.Addr.Resource @@ -400,7 +419,15 @@ func marshalResources(resources map[string]*states.Resource, module addrs.Module ret = append(ret, current) } - for deposedKey, rios := range ri.Deposed { + var sortedDeposedKeys []string + for k := range ri.Deposed { + sortedDeposedKeys = append(sortedDeposedKeys, string(k)) + } + sort.Strings(sortedDeposedKeys) + + for _, deposedKey := range sortedDeposedKeys { + rios := ri.Deposed[states.DeposedKey(deposedKey)] + // copy the base fields from the current instance deposed := Resource{ Address: current.Address, @@ -436,16 +463,12 @@ func marshalResources(resources map[string]*states.Resource, module addrs.Module if riObj.Status == states.ObjectTainted { deposed.Tainted = true } - deposed.DeposedKey = deposedKey.String() + deposed.DeposedKey = deposedKey ret = append(ret, deposed) } } } - sort.Slice(ret, func(i, j int) bool { - return ret[i].Address < ret[j].Address - }) - return ret, nil } diff --git a/internal/command/testdata/show-json/provider-aliasing-conflict/output.json b/internal/command/testdata/show-json/provider-aliasing-conflict/output.json index 6b7ee48c8d..b95a96f185 100644 --- a/internal/command/testdata/show-json/provider-aliasing-conflict/output.json +++ b/internal/command/testdata/show-json/provider-aliasing-conflict/output.json @@ -39,6 +39,27 @@ } }, "resource_changes": [ + { + "address": "test_instance.test", + "mode": "managed", + "type": "test_instance", + "name": "test", + "provider_name": "registry.terraform.io/hashicorp/test", + "change": { + "actions": [ + "create" + ], + "before": null, + "after": { + "ami": "foo" + }, + "after_unknown": { + "id": true + }, + "before_sensitive": false, + "after_sensitive": {} + } + }, { "address": "module.child.test_instance.test", "module_address": "module.child", @@ -60,27 +81,6 @@ "before_sensitive": false, "after_sensitive": {} } - }, - { - "address": "test_instance.test", - "mode": "managed", - "type": "test_instance", - "name": "test", - "provider_name": "registry.terraform.io/hashicorp/test", - "change": { - "actions": [ - "create" - ], - "before": null, - "after": { - "ami": "foo" - }, - "after_unknown": { - "id": true - }, - "before_sensitive": false, - "after_sensitive": {} - } } ], "configuration": { diff --git a/internal/command/testdata/show-json/provider-aliasing-default/output.json b/internal/command/testdata/show-json/provider-aliasing-default/output.json index d6b1706437..916d78bdd3 100644 --- a/internal/command/testdata/show-json/provider-aliasing-default/output.json +++ b/internal/command/testdata/show-json/provider-aliasing-default/output.json @@ -75,6 +75,49 @@ } }, "resource_changes": [ + { + "address": "test_instance.test", + "mode": "managed", + "type": "test_instance", + "name": "test", + "provider_name": "registry.terraform.io/hashicorp/test", + "change": { + "actions": [ + "create" + ], + "before": null, + "after": { + "ami": "foo" + }, + "after_unknown": { + "id": true + }, + "before_sensitive": false, + "after_sensitive": {} + } + }, + { + "address": "module.child.test_instance.test", + "module_address": "module.child", + "mode": "managed", + "type": "test_instance", + "name": "test", + "provider_name": "registry.terraform.io/hashicorp/test", + "change": { + "actions": [ + "create" + ], + "before": null, + "after": { + "ami": "bar" + }, + "after_unknown": { + "id": true + }, + "before_sensitive": false, + "after_sensitive": {} + } + }, { "address": "module.child.module.no_requirements.test_instance.test", "module_address": "module.child.module.no_requirements", @@ -118,49 +161,6 @@ "before_sensitive": false, "after_sensitive": {} } - }, - { - "address": "module.child.test_instance.test", - "module_address": "module.child", - "mode": "managed", - "type": "test_instance", - "name": "test", - "provider_name": "registry.terraform.io/hashicorp/test", - "change": { - "actions": [ - "create" - ], - "before": null, - "after": { - "ami": "bar" - }, - "after_unknown": { - "id": true - }, - "before_sensitive": false, - "after_sensitive": {} - } - }, - { - "address": "test_instance.test", - "mode": "managed", - "type": "test_instance", - "name": "test", - "provider_name": "registry.terraform.io/hashicorp/test", - "change": { - "actions": [ - "create" - ], - "before": null, - "after": { - "ami": "foo" - }, - "after_unknown": { - "id": true - }, - "before_sensitive": false, - "after_sensitive": {} - } } ], "configuration": { diff --git a/internal/command/testdata/show-json/provider-aliasing/output.json b/internal/command/testdata/show-json/provider-aliasing/output.json index 187141c9cf..6dc3b7447a 100755 --- a/internal/command/testdata/show-json/provider-aliasing/output.json +++ b/internal/command/testdata/show-json/provider-aliasing/output.json @@ -155,11 +155,10 @@ }, "resource_changes": [ { - "address": "module.child.module.grandchild.test_instance.test_alternate", - "module_address": "module.child.module.grandchild", + "address": "test_instance.test", "mode": "managed", "type": "test_instance", - "name": "test_alternate", + "name": "test", "provider_name": "registry.terraform.io/hashicorp/test", "change": { "actions": [ @@ -167,7 +166,7 @@ ], "before": null, "after": { - "ami": "secondary" + "ami": "foo" }, "after_unknown": { "id": true @@ -177,11 +176,10 @@ } }, { - "address": "module.child.module.grandchild.test_instance.test_main", - "module_address": "module.child.module.grandchild", + "address": "test_instance.test_backup", "mode": "managed", "type": "test_instance", - "name": "test_main", + "name": "test_backup", "provider_name": "registry.terraform.io/hashicorp/test", "change": { "actions": [ @@ -189,7 +187,7 @@ ], "before": null, "after": { - "ami": "main" + "ami": "foo-backup" }, "after_unknown": { "id": true @@ -242,50 +240,6 @@ "after_sensitive": {} } }, - { - "address": "module.sibling.module.grandchild.test_instance.test_alternate", - "module_address": "module.sibling.module.grandchild", - "mode": "managed", - "type": "test_instance", - "name": "test_alternate", - "provider_name": "registry.terraform.io/hashicorp/test", - "change": { - "actions": [ - "create" - ], - "before": null, - "after": { - "ami": "secondary" - }, - "after_unknown": { - "id": true - }, - "before_sensitive": false, - "after_sensitive": {} - } - }, - { - "address": "module.sibling.module.grandchild.test_instance.test_main", - "module_address": "module.sibling.module.grandchild", - "mode": "managed", - "type": "test_instance", - "name": "test_main", - "provider_name": "registry.terraform.io/hashicorp/test", - "change": { - "actions": [ - "create" - ], - "before": null, - "after": { - "ami": "main" - }, - "after_unknown": { - "id": true - }, - "before_sensitive": false, - "after_sensitive": {} - } - }, { "address": "module.sibling.test_instance.test_primary", "module_address": "module.sibling", @@ -331,10 +285,11 @@ } }, { - "address": "test_instance.test", + "address": "module.child.module.grandchild.test_instance.test_alternate", + "module_address": "module.child.module.grandchild", "mode": "managed", "type": "test_instance", - "name": "test", + "name": "test_alternate", "provider_name": "registry.terraform.io/hashicorp/test", "change": { "actions": [ @@ -342,7 +297,7 @@ ], "before": null, "after": { - "ami": "foo" + "ami": "secondary" }, "after_unknown": { "id": true @@ -352,10 +307,11 @@ } }, { - "address": "test_instance.test_backup", + "address": "module.child.module.grandchild.test_instance.test_main", + "module_address": "module.child.module.grandchild", "mode": "managed", "type": "test_instance", - "name": "test_backup", + "name": "test_main", "provider_name": "registry.terraform.io/hashicorp/test", "change": { "actions": [ @@ -363,7 +319,51 @@ ], "before": null, "after": { - "ami": "foo-backup" + "ami": "main" + }, + "after_unknown": { + "id": true + }, + "before_sensitive": false, + "after_sensitive": {} + } + }, + { + "address": "module.sibling.module.grandchild.test_instance.test_alternate", + "module_address": "module.sibling.module.grandchild", + "mode": "managed", + "type": "test_instance", + "name": "test_alternate", + "provider_name": "registry.terraform.io/hashicorp/test", + "change": { + "actions": [ + "create" + ], + "before": null, + "after": { + "ami": "secondary" + }, + "after_unknown": { + "id": true + }, + "before_sensitive": false, + "after_sensitive": {} + } + }, + { + "address": "module.sibling.module.grandchild.test_instance.test_main", + "module_address": "module.sibling.module.grandchild", + "mode": "managed", + "type": "test_instance", + "name": "test_main", + "provider_name": "registry.terraform.io/hashicorp/test", + "change": { + "actions": [ + "create" + ], + "before": null, + "after": { + "ami": "main" }, "after_unknown": { "id": true