Merge pull request #12 from Pix4D/pci-1828-fix-more-complicated-match

Pci 1828 fix more complicated match

Taskfile.yml

@@ -8,25 +8,18 @@ tasks:
   default:
     deps: [test]
 
-  clean:
-    desc: Delete build artifacts
-    cmds: [rm -rf bin/*]
-
-  terravalet:
+  build:
     desc: Build the terravalet executable
-    dir: bin
     cmds:
-      - go build -v -ldflags="{{.LDFLAGS}}" ..
+      - go build -o bin/terravalet -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
-    deps: [terravalet]
+    deps: [build]
     cmds:
       - "{{.TESTRUNNER}} ./..."
     vars:

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() {
@@ -132,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)
@@ -344,54 +346,56 @@ 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 })
+
+	for len(candidates) > 0 {
+		bestCandidate := candidates[0]
+		tmpCandidates := []candidate{}
+
+		for _, c := range candidates[1:] {
+			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,
+						)
+				}
+			}
+			if (bestCandidate.create != c.create) && (bestCandidate.destroy != c.destroy) {
+				tmpCandidates = append(tmpCandidates, candidate{c.distance, c.create, c.destroy})
+			}
 
-	// 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
+		}
 
-		// Remove matched elements from the two sets.
-		destroy.Remove(v.word)
-		create.Remove(k)
+		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

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 {
@@ -437,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)
@@ -454,3 +464,18 @@ func TestMatchFuzzyZeroUnmatched(t *testing.T) {
 		})
 	}
 }
+
+func TestMatchFuzzyError(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`)
+		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() != wantError {
+			t.Fatalf("got: %s; want: %s", err.Error(), wantError)
+		}
+	})
+}

testdata/07_fuzzy-match.down.sh

@@ -0,0 +1,28 @@
+#! /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'
+

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

testdata/07_fuzzy-match.up.sh

@@ -0,0 +1,28 @@
+#! /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'
+