From 1fdfdb65d42577ab6dbcc4bccef1182320d9732d Mon Sep 17 00:00:00 2001 From: aliculPix4D Date: Thu, 22 Jul 2021 11:31:59 +0200 Subject: [PATCH 1/4] PCI-1828 Remove unused shortversion var and fix Taskfile --- Taskfile.yml | 9 ++++----- main.go | 3 +-- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/Taskfile.yml b/Taskfile.yml index 35a0ed3..8da6e9c 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -14,15 +14,14 @@ tasks: terravalet: desc: Build the terravalet executable - dir: bin + deps: [clean] cmds: - - go build -v -ldflags="{{.LDFLAGS}}" .. + - mkdir -p bin + - cd bin && go build -v -ldflags="{{.LDFLAGS}}" .. vars: &build-vars FULL_VERSION: sh: git describe --long --dirty --always - SHORT_VERSION: - sh: git describe --abbrev=0 - LDFLAGS: -w -s -X main.fullVersion={{.FULL_VERSION}} -X main.shortVersion={{.SHORT_VERSION}} + LDFLAGS: -w -s -X main.fullVersion={{.FULL_VERSION}} test: desc: Run the integration tests diff --git a/main.go b/main.go index 14a065a..3bfd67f 100644 --- a/main.go +++ b/main.go @@ -20,8 +20,7 @@ import ( var ( // Filled by the linker. - fullVersion = "unknown" // example: v0.0.9-8-g941583d027-dirty - shortVersion = "unknown" // example: v0.0.9 + fullVersion = "unknown" // example: v0.0.9-8-g941583d027-dirty ) func main() { From 223b6db426626dae6b0c666b7c4caf8eaa82aefe Mon Sep 17 00:00:00 2001 From: aliculPix4D Date: Thu, 22 Jul 2021 11:34:31 +0200 Subject: [PATCH 2/4] PCI-1828 Add regression test and fixtures --- main_test.go | 7 +++++++ testdata/07_fuzzy-match.down.sh | 27 +++++++++++++++++++++++++++ testdata/07_fuzzy-match.plan.txt | 13 +++++++++++++ testdata/07_fuzzy-match.up.sh | 27 +++++++++++++++++++++++++++ 4 files changed, 74 insertions(+) create mode 100644 testdata/07_fuzzy-match.down.sh create mode 100644 testdata/07_fuzzy-match.plan.txt create mode 100644 testdata/07_fuzzy-match.up.sh diff --git a/main_test.go b/main_test.go index d63ce36..2ea9701 100644 --- a/main_test.go +++ b/main_test.go @@ -44,6 +44,13 @@ func TestRunRenameSuccess(t *testing.T) { "testdata/03_fuzzy-match.up.sh", "testdata/03_fuzzy-match.down.sh", }, + { + "q-gram fuzzy match complicated (regression)", + []string{"-fuzzy-match"}, + "testdata/07_fuzzy-match.plan.txt", + "testdata/07_fuzzy-match.up.sh", + "testdata/07_fuzzy-match.down.sh", + }, } for _, tc := range testCases { diff --git a/testdata/07_fuzzy-match.down.sh b/testdata/07_fuzzy-match.down.sh new file mode 100644 index 0000000..4148761 --- /dev/null +++ b/testdata/07_fuzzy-match.down.sh @@ -0,0 +1,27 @@ +#! /usr/bin/sh +# DO NOT EDIT. Generated by terravalet. +# terravalet_output_format=2 +# +# This script will move 5 items. + +set -e + +terraform state mv -lock=false -state=local.tfstate \ + 'module.prometheus_instance.aws_instance.instance' \ + 'module.prometheus.aws_instance.prometheus' + +terraform state mv -lock=false -state=local.tfstate \ + 'module.prometheus_instance.aws_route53_record.internal' \ + 'aws_route53_record.private["prometheus"]' + +terraform state mv -lock=false -state=local.tfstate \ + 'module.prometheus_instance.aws_security_group_rule.extra_rules["reverseproxy_to_prometheus_pushprox"]' \ + 'aws_security_group_rule.reverseproxy_to_prometheus_pushprox' + +terraform state mv -lock=false -state=local.tfstate \ + 'module.prometheus_instance.aws_volume_attachment.volumes["/dev/xvdh"]' \ + 'module.prometheus.aws_volume_attachment.storage_attach' + +terraform state mv -lock=false -state=local.tfstate \ + 'module.prometheus_instance.null_resource.provision' \ + 'module.prometheus.null_resource.provision_prometheus' diff --git a/testdata/07_fuzzy-match.plan.txt b/testdata/07_fuzzy-match.plan.txt new file mode 100644 index 0000000..eaa091a --- /dev/null +++ b/testdata/07_fuzzy-match.plan.txt @@ -0,0 +1,13 @@ +# aws_route53_record.private["prometheus"] will be destroyed +# aws_security_group_rule.reverseproxy_to_prometheus_pushprox will be destroyed +# null_resource.configure_alerts will be created +# null_resource.provision_monitor_in_nginx must be replaced +# module.prometheus.aws_instance.prometheus will be destroyed +# module.prometheus.aws_volume_attachment.storage_attach will be destroyed +# module.prometheus.null_resource.configure_alerts will be destroyed +# module.prometheus.null_resource.provision_prometheus will be destroyed +# module.prometheus_instance.aws_instance.instance will be created +# module.prometheus_instance.aws_route53_record.internal will be created +# module.prometheus_instance.aws_security_group_rule.extra_rules["reverseproxy_to_prometheus_pushprox"] will be created +# module.prometheus_instance.aws_volume_attachment.volumes["/dev/xvdh"] will be created +# module.prometheus_instance.null_resource.provision will be created diff --git a/testdata/07_fuzzy-match.up.sh b/testdata/07_fuzzy-match.up.sh new file mode 100644 index 0000000..f17d0e0 --- /dev/null +++ b/testdata/07_fuzzy-match.up.sh @@ -0,0 +1,27 @@ +#! /usr/bin/sh +# DO NOT EDIT. Generated by terravalet. +# terravalet_output_format=2 +# +# This script will move 5 items. + +set -e + +terraform state mv -lock=false -state=local.tfstate \ + 'aws_route53_record.private["prometheus"]' \ + 'module.prometheus_instance.aws_route53_record.internal' + +terraform state mv -lock=false -state=local.tfstate \ + 'aws_security_group_rule.reverseproxy_to_prometheus_pushprox' \ + 'module.prometheus_instance.aws_security_group_rule.extra_rules["reverseproxy_to_prometheus_pushprox"]' + +terraform state mv -lock=false -state=local.tfstate \ + 'module.prometheus.aws_instance.prometheus' \ + 'module.prometheus_instance.aws_instance.instance' + +terraform state mv -lock=false -state=local.tfstate \ + 'module.prometheus.aws_volume_attachment.storage_attach' \ + 'module.prometheus_instance.aws_volume_attachment.volumes["/dev/xvdh"]' + +terraform state mv -lock=false -state=local.tfstate \ + 'module.prometheus.null_resource.provision_prometheus' \ + 'module.prometheus_instance.null_resource.provision' From d67a002c57293b0c4074eeab71d1081e76579b0a Mon Sep 17 00:00:00 2001 From: aliculPix4D Date: Thu, 22 Jul 2021 11:41:44 +0200 Subject: [PATCH 3/4] PCI-1828 Fix matchFuzzy() algorithm --- main.go | 71 ++++++++++++++++++--------------- main_test.go | 17 +++++++- testdata/07_fuzzy-match.down.sh | 1 + testdata/07_fuzzy-match.up.sh | 1 + 4 files changed, 57 insertions(+), 33 deletions(-) diff --git a/main.go b/main.go index 3bfd67f..ca5931a 100644 --- a/main.go +++ b/main.go @@ -131,7 +131,10 @@ func rename(upPath, downPath, planPath, localStatePath string, fuzzyMatch bool) return fmt.Errorf("required fuzzy-match but there is nothing left to match") } if fuzzyMatch { - upMatches, downMatches = matchFuzzy(create, destroy) + upMatches, downMatches, err = matchFuzzy(create, destroy) + if err != nil { + return fmt.Errorf("fuzzyMatch: %v", err) + } msg := collectErrors(create, destroy) if msg != "" { return fmt.Errorf("matchFuzzy: %v", msg) @@ -343,54 +346,58 @@ func matchExact(create, destroy *strset.Set) (map[string]string, map[string]stri // The criterium used to perform a matchFuzzy is that one of the two elements must be a // fuzzy match of the other, according to some definition of fuzzy. // Note that the longest element could be the old or the new one, it depends on the inputs. -func matchFuzzy(create, destroy *strset.Set) (map[string]string, map[string]string) { +func matchFuzzy(create, destroy *strset.Set) (map[string]string, map[string]string, error) { // old -> new (or equvalenty: destroy -> create) upMatches := map[string]string{} downMatches := map[string]string{} type candidate struct { - word string distance int + create string + destroy string } - reverse := map[string]candidate{} - - destroyL := destroy.List() - sort.Strings(destroyL) - createL := create.List() - sort.Strings(createL) + candidates := []candidate{} - for _, d := range destroyL { - for _, c := range createL { + for _, d := range destroy.List() { + for _, c := range create.List() { // Here we could also use a custom NGramSizes via // stringosim.QGramSimilarityOptions dist := stringosim.QGram([]rune(d), []rune(c)) - curr, ok := reverse[c] - if !ok || dist < curr.distance { - reverse[c] = candidate{d, dist} - } + candidates = append(candidates, candidate{dist, c, d}) } } + sort.Slice(candidates, func(i, j int) bool { return candidates[i].distance < candidates[j].distance }) - // Sort the reverse index for reproducibility - keys := make([]string, 0, len(reverse)) - for k := range reverse { - keys = append(keys, k) - } - sort.Strings(keys) - // Traverse the reverse index, populate the up and down match maps and update the source sets. - fmt.Printf("WARNING fuzzy match enabled. Double-check the following matches:\n") - for _, k := range keys { - v := reverse[k] - fmt.Printf("%2d %-50s -> %s\n", v.distance, v.word, k) - upMatches[v.word] = k - downMatches[k] = v.word + for len(candidates) > 0 { + bestCandidate := candidates[0] - // Remove matched elements from the two sets. - destroy.Remove(v.word) - create.Remove(k) + for _, c := range candidates[1:] { + if bestCandidate.distance < c.distance { + break + } + if (bestCandidate.create == c.create) || (bestCandidate.destroy == c.destroy) { + return map[string]string{}, + map[string]string{}, + fmt.Errorf("ambiguous migration: {%s} -> {%s} or {%s} -> {%s}", + bestCandidate.create, bestCandidate.destroy, + c.create, c.destroy, + ) + } + } + tmpCandidates := []candidate{} + for _, c := range candidates[1:] { + if (bestCandidate.create != c.create) && (bestCandidate.destroy != c.destroy) { + tmpCandidates = append(tmpCandidates, candidate{c.distance, c.create, c.destroy}) + } + } + candidates = tmpCandidates + upMatches[bestCandidate.destroy] = bestCandidate.create + downMatches[bestCandidate.create] = bestCandidate.destroy + destroy.Remove(bestCandidate.destroy) + create.Remove(bestCandidate.create) } - return upMatches, downMatches + return upMatches, downMatches, nil } // Given a map old->new, create a script that for each element in the map issues the diff --git a/main_test.go b/main_test.go index 2ea9701..874ea66 100644 --- a/main_test.go +++ b/main_test.go @@ -444,7 +444,7 @@ func TestMatchFuzzyZeroUnmatched(t *testing.T) { for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { - gotUpMatches, gotDownMatches := matchFuzzy(tc.create, tc.destroy) + gotUpMatches, gotDownMatches, _ := matchFuzzy(tc.create, tc.destroy) if diff := cmp.Diff(tc.wantUpMatches, gotUpMatches); diff != "" { t.Errorf("\nupMatches: mismatch (-want +got):\n%s", diff) @@ -461,3 +461,18 @@ func TestMatchFuzzyZeroUnmatched(t *testing.T) { }) } } + +func TestMatchFuzzyError(t *testing.T) { + t.Run("ambiguous migration: two differnt items have the same match", func(t *testing.T) { + create := set.NewStringSet(`abcde`, `abdecde`) + destroy := set.NewStringSet(`abdcde`, `hfjabd`) + expectedError := "ambiguous migration: {abcde} -> {abdcde} or {abdecde} -> {abdcde}" + _, _, err := matchFuzzy(create, destroy) + if err == nil { + t.Fatalf("got: no error; want: an ambiguous migration error") + } + if err.Error() != expectedError { + t.Fatalf("got: %s; want: %s", err.Error(), expectedError) + } + }) +} diff --git a/testdata/07_fuzzy-match.down.sh b/testdata/07_fuzzy-match.down.sh index 4148761..fd04979 100644 --- a/testdata/07_fuzzy-match.down.sh +++ b/testdata/07_fuzzy-match.down.sh @@ -25,3 +25,4 @@ terraform state mv -lock=false -state=local.tfstate \ terraform state mv -lock=false -state=local.tfstate \ 'module.prometheus_instance.null_resource.provision' \ 'module.prometheus.null_resource.provision_prometheus' + diff --git a/testdata/07_fuzzy-match.up.sh b/testdata/07_fuzzy-match.up.sh index f17d0e0..4963267 100644 --- a/testdata/07_fuzzy-match.up.sh +++ b/testdata/07_fuzzy-match.up.sh @@ -25,3 +25,4 @@ terraform state mv -lock=false -state=local.tfstate \ terraform state mv -lock=false -state=local.tfstate \ 'module.prometheus.null_resource.provision_prometheus' \ 'module.prometheus_instance.null_resource.provision' + From 9faec0b633d94dd9a8fa1127032fbb385ca7c784 Mon Sep 17 00:00:00 2001 From: aliculPix4D Date: Thu, 22 Jul 2021 12:00:49 +0200 Subject: [PATCH 4/4] PCI-1828 Address review comments --- Taskfile.yml | 12 +++--------- main.go | 24 +++++++++++------------- main_test.go | 13 ++++++++----- 3 files changed, 22 insertions(+), 27 deletions(-) diff --git a/Taskfile.yml b/Taskfile.yml index 8da6e9c..91c3e19 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -8,16 +8,10 @@ tasks: default: deps: [test] - clean: - desc: Delete build artifacts - cmds: [rm -rf bin/*] - - terravalet: + build: desc: Build the terravalet executable - deps: [clean] cmds: - - mkdir -p bin - - cd bin && go build -v -ldflags="{{.LDFLAGS}}" .. + - go build -o bin/terravalet -v -ldflags="{{.LDFLAGS}}" . vars: &build-vars FULL_VERSION: sh: git describe --long --dirty --always @@ -25,7 +19,7 @@ tasks: test: desc: Run the integration tests - deps: [terravalet] + deps: [build] cmds: - "{{.TESTRUNNER}} ./..." vars: diff --git a/main.go b/main.go index ca5931a..aca7230 100644 --- a/main.go +++ b/main.go @@ -370,26 +370,24 @@ func matchFuzzy(create, destroy *strset.Set) (map[string]string, map[string]stri for len(candidates) > 0 { bestCandidate := candidates[0] + tmpCandidates := []candidate{} for _, c := range candidates[1:] { - if bestCandidate.distance < c.distance { - break - } - if (bestCandidate.create == c.create) || (bestCandidate.destroy == c.destroy) { - return map[string]string{}, - map[string]string{}, - fmt.Errorf("ambiguous migration: {%s} -> {%s} or {%s} -> {%s}", - bestCandidate.create, bestCandidate.destroy, - c.create, c.destroy, - ) + if bestCandidate.distance == c.distance { + if (bestCandidate.create == c.create) || (bestCandidate.destroy == c.destroy) { + return map[string]string{}, map[string]string{}, + fmt.Errorf("ambiguous migration: {%s} -> {%s} or {%s} -> {%s}", + bestCandidate.create, bestCandidate.destroy, + c.create, c.destroy, + ) + } } - } - tmpCandidates := []candidate{} - for _, c := range candidates[1:] { if (bestCandidate.create != c.create) && (bestCandidate.destroy != c.destroy) { tmpCandidates = append(tmpCandidates, candidate{c.distance, c.create, c.destroy}) } + } + candidates = tmpCandidates upMatches[bestCandidate.destroy] = bestCandidate.create downMatches[bestCandidate.create] = bestCandidate.destroy diff --git a/main_test.go b/main_test.go index 874ea66..361d68b 100644 --- a/main_test.go +++ b/main_test.go @@ -444,7 +444,10 @@ func TestMatchFuzzyZeroUnmatched(t *testing.T) { for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { - gotUpMatches, gotDownMatches, _ := matchFuzzy(tc.create, tc.destroy) + gotUpMatches, gotDownMatches, err := matchFuzzy(tc.create, tc.destroy) + if err != nil { + t.Fatalf("got: %s; want: no error", err) + } if diff := cmp.Diff(tc.wantUpMatches, gotUpMatches); diff != "" { t.Errorf("\nupMatches: mismatch (-want +got):\n%s", diff) @@ -463,16 +466,16 @@ func TestMatchFuzzyZeroUnmatched(t *testing.T) { } func TestMatchFuzzyError(t *testing.T) { - t.Run("ambiguous migration: two differnt items have the same match", func(t *testing.T) { + t.Run("ambiguous migration: two different items have the same match", func(t *testing.T) { create := set.NewStringSet(`abcde`, `abdecde`) destroy := set.NewStringSet(`abdcde`, `hfjabd`) - expectedError := "ambiguous migration: {abcde} -> {abdcde} or {abdecde} -> {abdcde}" + wantError := "ambiguous migration: {abcde} -> {abdcde} or {abdecde} -> {abdcde}" _, _, err := matchFuzzy(create, destroy) if err == nil { t.Fatalf("got: no error; want: an ambiguous migration error") } - if err.Error() != expectedError { - t.Fatalf("got: %s; want: %s", err.Error(), expectedError) + if err.Error() != wantError { + t.Fatalf("got: %s; want: %s", err.Error(), wantError) } }) }