Skip to content

Commit 9086688

Browse files
fwieselmumoshu
fwiesel
authored and
mumoshu
committed
Discover renames based on content
If we rename objects, they will appear as add and remove actions, which makes it difficult to assess the semantic delta, which may only be a simple renaming. The current implementation creates a diff between every added & removeed item of the same kind, which means at worst O(m*n) runtime. So it should only be enabled if such changes are rare
1 parent d70a28c commit 9086688

File tree

3 files changed

+229
-13
lines changed

3 files changed

+229
-13
lines changed

‎cmd/options.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,18 @@ import (
55
"github.com/spf13/pflag"
66
)
77

8+
// AddDiffOptions adds flags for the various consolidated options to the functions in the diff package
89
func AddDiffOptions(f *pflag.FlagSet, o *diff.Options) {
910
f.BoolP("suppress-secrets", "q", false, "suppress secrets in the output")
1011
f.BoolVar(&o.ShowSecrets, "show-secrets", false, "do not redact secret values in the output")
1112
f.StringArrayVar(&o.SuppressedKinds, "suppress", []string{}, "allows suppression of the values listed in the diff output")
1213
f.IntVarP(&o.OutputContext, "context", "C", -1, "output NUM lines of context around changes")
1314
f.StringVar(&o.OutputFormat, "output", "diff", "Possible values: diff, simple, template. When set to \"template\", use the env var HELM_DIFF_TPL to specify the template.")
1415
f.BoolVar(&o.StripTrailingCR, "strip-trailing-cr", false, "strip trailing carriage return on input")
16+
f.Float32VarP(&o.FindRenames, "find-renames", "D", 0, "Enable rename detection if set to any value greater than 0. If specified, the value denotes the maximum fraction of changed content as lines added + removed compared to total lines in a diff for considering it a rename. Only objects of the same Kind are attempted to be matched")
1517
}
1618

19+
// ProcessDiffOptions processes the set flags and handles possible interactions between them
1720
func ProcessDiffOptions(f *pflag.FlagSet, o *diff.Options) {
1821
if q, _ := f.GetBool("suppress-secrets"); q {
1922
o.SuppressedKinds = append(o.SuppressedKinds, "Secret")

‎diff/diff.go

Lines changed: 64 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,32 +25,45 @@ type Options struct {
2525
StripTrailingCR bool
2626
ShowSecrets bool
2727
SuppressedKinds []string
28+
FindRenames float32
2829
}
2930

3031
// Manifests diff on manifests
3132
func Manifests(oldIndex, newIndex map[string]*manifest.MappingResult, options *Options, to io.Writer) bool {
3233
report := Report{}
3334
report.setupReportFormat(options.OutputFormat)
35+
var possiblyRemoved []string
36+
3437
for _, key := range sortedKeys(oldIndex) {
3538
oldContent := oldIndex[key]
3639

3740
if newContent, ok := newIndex[key]; ok {
3841
// modified?
3942
doDiff(&report, key, oldContent, newContent, options)
4043
} else {
41-
// removed
42-
doDiff(&report, key, oldContent, nil, options)
44+
possiblyRemoved = append(possiblyRemoved, key)
4345
}
4446
}
4547

48+
var possiblyAdded []string
4649
for _, key := range sortedKeys(newIndex) {
47-
newContent := newIndex[key]
48-
4950
if _, ok := oldIndex[key]; !ok {
50-
doDiff(&report, key, nil, newContent, options)
51+
possiblyAdded = append(possiblyAdded, key)
5152
}
5253
}
5354

55+
removed, added := contentSearch(&report, possiblyRemoved, oldIndex, possiblyAdded, newIndex, options)
56+
57+
for _, key := range removed {
58+
oldContent := oldIndex[key]
59+
doDiff(&report, key, oldContent, nil, options)
60+
}
61+
62+
for _, key := range added {
63+
newContent := newIndex[key]
64+
doDiff(&report, key, nil, newContent, options)
65+
}
66+
5467
seenAnyChanges := len(report.entries) > 0
5568
report.print(to)
5669
report.clean()
@@ -67,6 +80,52 @@ func actualChanges(diff []difflib.DiffRecord) int {
6780
return changes
6881
}
6982

83+
func contentSearch(report *Report, possiblyRemoved []string, oldIndex map[string]*manifest.MappingResult, possiblyAdded []string, newIndex map[string]*manifest.MappingResult, options *Options) ([]string, []string) {
84+
if options.FindRenames <= 0 {
85+
return possiblyRemoved, possiblyAdded
86+
}
87+
88+
var removed []string
89+
90+
for _, removedKey := range possiblyRemoved {
91+
oldContent := oldIndex[removedKey]
92+
var smallestKey string
93+
var smallestFraction float32 = math.MaxFloat32
94+
for _, addedKey := range possiblyAdded {
95+
newContent := newIndex[addedKey]
96+
if oldContent.Kind != newContent.Kind {
97+
continue
98+
}
99+
100+
if !options.ShowSecrets {
101+
redactSecrets(oldContent, newContent)
102+
}
103+
104+
diff := diffMappingResults(oldContent, newContent, options.StripTrailingCR)
105+
delta := actualChanges(diff)
106+
if delta == 0 || len(diff) == 0 {
107+
continue // Should never happen, but better safe than sorry
108+
}
109+
fraction := float32(delta) / float32(len(diff))
110+
if fraction > 0 && fraction < smallestFraction {
111+
smallestKey = addedKey
112+
smallestFraction = fraction
113+
}
114+
}
115+
116+
if smallestFraction < options.FindRenames {
117+
index := sort.SearchStrings(possiblyAdded, smallestKey)
118+
possiblyAdded = append(possiblyAdded[:index], possiblyAdded[index+1:]...)
119+
newContent := newIndex[smallestKey]
120+
doDiff(report, removedKey, oldContent, newContent, options)
121+
} else {
122+
removed = append(removed, removedKey)
123+
}
124+
}
125+
126+
return removed, possiblyAdded
127+
}
128+
70129
func doDiff(report *Report, key string, oldContent *manifest.MappingResult, newContent *manifest.MappingResult, options *Options) {
71130
if oldContent != nil && newContent != nil && oldContent.Content == newContent.Content {
72131
return

‎diff/diff_test.go

Lines changed: 162 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -195,9 +195,72 @@ metadata:
195195
`,
196196
}}
197197

198+
specReleaseSpec := map[string]*manifest.MappingResult{
199+
"default, nginx, Deployment (apps)": {
200+
201+
Name: "default, nginx, Deployment (apps)",
202+
Kind: "Deployment",
203+
Content: `
204+
apiVersion: apps/v1
205+
kind: Deployment
206+
metadata:
207+
name: nginx
208+
spec:
209+
replicas: 3
210+
`,
211+
}}
212+
213+
specReleaseRenamed := map[string]*manifest.MappingResult{
214+
"default, nginx-renamed, Deployment (apps)": {
215+
216+
Name: "default, nginx-renamed, Deployment (apps)",
217+
Kind: "Deployment",
218+
Content: `
219+
apiVersion: apps/v1
220+
kind: Deployment
221+
metadata:
222+
name: nginx-renamed
223+
spec:
224+
replicas: 3
225+
`,
226+
}}
227+
228+
specReleaseRenamedAndUpdated := map[string]*manifest.MappingResult{
229+
"default, nginx-renamed, Deployment (apps)": {
230+
231+
Name: "default, nginx-renamed, Deployment (apps)",
232+
Kind: "Deployment",
233+
Content: `
234+
apiVersion: apps/v1
235+
kind: Deployment
236+
metadata:
237+
name: nginx-renamed
238+
spec:
239+
replicas: 1
240+
`,
241+
}}
242+
243+
specReleaseRenamedAndAdded := map[string]*manifest.MappingResult{
244+
"default, nginx-renamed, Deployment (apps)": {
245+
246+
Name: "default, nginx-renamed, Deployment (apps)",
247+
Kind: "Deployment",
248+
Content: `
249+
apiVersion: apps/v1
250+
kind: Deployment
251+
metadata:
252+
name: nginx-renamed
253+
spec:
254+
replicas: 3
255+
selector:
256+
matchLabels:
257+
app: nginx-renamed
258+
`,
259+
}}
260+
198261
t.Run("OnChange", func(t *testing.T) {
199262
var buf1 bytes.Buffer
200-
diffOptions := Options{"diff", 10, false, true, []string{}}
263+
diffOptions := Options{"diff", 10, false, true, []string{}, 0.0}
201264

202265
if changesSeen := Manifests(specBeta, specRelease, &diffOptions, &buf1); !changesSeen {
203266
t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`")
@@ -211,12 +274,103 @@ metadata:
211274
metadata:
212275
name: nginx
213276
277+
`, buf1.String())
278+
})
279+
280+
t.Run("OnChangeRename", func(t *testing.T) {
281+
var buf1 bytes.Buffer
282+
diffOptions := Options{"diff", 10, false, true, []string{}, 0.5}
283+
284+
if changesSeen := Manifests(specReleaseSpec, specReleaseRenamed, &diffOptions, &buf1); !changesSeen {
285+
t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`")
286+
}
287+
288+
require.Equal(t, `default, nginx, Deployment (apps) has changed:
289+
290+
apiVersion: apps/v1
291+
kind: Deployment
292+
metadata:
293+
- name: nginx
294+
+ name: nginx-renamed
295+
spec:
296+
replicas: 3
297+
298+
`, buf1.String())
299+
})
300+
301+
t.Run("OnChangeRenameAndUpdate", func(t *testing.T) {
302+
var buf1 bytes.Buffer
303+
diffOptions := Options{"diff", 10, false, true, []string{}, 0.5}
304+
305+
if changesSeen := Manifests(specReleaseSpec, specReleaseRenamedAndUpdated, &diffOptions, &buf1); !changesSeen {
306+
t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`")
307+
}
308+
309+
require.Equal(t, `default, nginx, Deployment (apps) has changed:
310+
311+
apiVersion: apps/v1
312+
kind: Deployment
313+
metadata:
314+
- name: nginx
315+
+ name: nginx-renamed
316+
spec:
317+
- replicas: 3
318+
+ replicas: 1
319+
320+
`, buf1.String())
321+
})
322+
323+
t.Run("OnChangeRenameAndAdded", func(t *testing.T) {
324+
var buf1 bytes.Buffer
325+
diffOptions := Options{"diff", 10, false, true, []string{}, 0.5}
326+
327+
if changesSeen := Manifests(specReleaseSpec, specReleaseRenamedAndAdded, &diffOptions, &buf1); !changesSeen {
328+
t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`")
329+
}
330+
331+
require.Equal(t, `default, nginx, Deployment (apps) has changed:
332+
333+
apiVersion: apps/v1
334+
kind: Deployment
335+
metadata:
336+
- name: nginx
337+
+ name: nginx-renamed
338+
spec:
339+
replicas: 3
340+
+ selector:
341+
+ matchLabels:
342+
+ app: nginx-renamed
343+
344+
`, buf1.String())
345+
})
346+
347+
t.Run("OnChangeRenameAndRemoved", func(t *testing.T) {
348+
var buf1 bytes.Buffer
349+
diffOptions := Options{"diff", 10, false, true, []string{}, 0.5}
350+
351+
if changesSeen := Manifests(specReleaseRenamedAndAdded, specReleaseSpec, &diffOptions, &buf1); !changesSeen {
352+
t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`")
353+
}
354+
355+
require.Equal(t, `default, nginx-renamed, Deployment (apps) has changed:
356+
357+
apiVersion: apps/v1
358+
kind: Deployment
359+
metadata:
360+
- name: nginx-renamed
361+
+ name: nginx
362+
spec:
363+
replicas: 3
364+
- selector:
365+
- matchLabels:
366+
- app: nginx-renamed
367+
214368
`, buf1.String())
215369
})
216370

217371
t.Run("OnNoChange", func(t *testing.T) {
218372
var buf2 bytes.Buffer
219-
diffOptions := Options{"diff", 10, false, true, []string{}}
373+
diffOptions := Options{"diff", 10, false, true, []string{}, 0.0}
220374

221375
if changesSeen := Manifests(specRelease, specRelease, &diffOptions, &buf2); changesSeen {
222376
t.Error("Unexpected return value from Manifests: Expected the return value to be `false` to indicate that it has NOT seen any change(s), but was `true`")
@@ -227,7 +381,7 @@ metadata:
227381

228382
t.Run("OnChangeSimple", func(t *testing.T) {
229383
var buf1 bytes.Buffer
230-
diffOptions := Options{"simple", 10, false, true, []string{}}
384+
diffOptions := Options{"simple", 10, false, true, []string{}, 0.0}
231385

232386
if changesSeen := Manifests(specBeta, specRelease, &diffOptions, &buf1); !changesSeen {
233387
t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`")
@@ -240,7 +394,7 @@ Plan: 0 to add, 1 to change, 0 to destroy.
240394

241395
t.Run("OnNoChangeSimple", func(t *testing.T) {
242396
var buf2 bytes.Buffer
243-
diffOptions := Options{"simple", 10, false, true, []string{}}
397+
diffOptions := Options{"simple", 10, false, true, []string{}, 0.0}
244398
if changesSeen := Manifests(specRelease, specRelease, &diffOptions, &buf2); changesSeen {
245399
t.Error("Unexpected return value from Manifests: Expected the return value to be `false` to indicate that it has NOT seen any change(s), but was `true`")
246400
}
@@ -250,7 +404,7 @@ Plan: 0 to add, 1 to change, 0 to destroy.
250404

251405
t.Run("OnChangeTemplate", func(t *testing.T) {
252406
var buf1 bytes.Buffer
253-
diffOptions := Options{"template", 10, false, true, []string{}}
407+
diffOptions := Options{"template", 10, false, true, []string{}, 0.0}
254408

255409
if changesSeen := Manifests(specBeta, specRelease, &diffOptions, &buf1); !changesSeen {
256410
t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`")
@@ -268,7 +422,7 @@ Plan: 0 to add, 1 to change, 0 to destroy.
268422

269423
t.Run("OnChangeJSON", func(t *testing.T) {
270424
var buf1 bytes.Buffer
271-
diffOptions := Options{"json", 10, false, true, []string{}}
425+
diffOptions := Options{"json", 10, false, true, []string{}, 0.0}
272426

273427
if changesSeen := Manifests(specBeta, specRelease, &diffOptions, &buf1); !changesSeen {
274428
t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`")
@@ -286,7 +440,7 @@ Plan: 0 to add, 1 to change, 0 to destroy.
286440

287441
t.Run("OnNoChangeTemplate", func(t *testing.T) {
288442
var buf2 bytes.Buffer
289-
diffOptions := Options{"template", 10, false, true, []string{}}
443+
diffOptions := Options{"template", 10, false, true, []string{}, 0.0}
290444

291445
if changesSeen := Manifests(specRelease, specRelease, &diffOptions, &buf2); changesSeen {
292446
t.Error("Unexpected return value from Manifests: Expected the return value to be `false` to indicate that it has NOT seen any change(s), but was `true`")
@@ -298,7 +452,7 @@ Plan: 0 to add, 1 to change, 0 to destroy.
298452
t.Run("OnChangeCustomTemplate", func(t *testing.T) {
299453
var buf1 bytes.Buffer
300454
os.Setenv("HELM_DIFF_TPL", "testdata/customTemplate.tpl")
301-
diffOptions := Options{"template", 10, false, true, []string{}}
455+
diffOptions := Options{"template", 10, false, true, []string{}, 0.0}
302456

303457
if changesSeen := Manifests(specBeta, specRelease, &diffOptions, &buf1); !changesSeen {
304458
t.Error("Unexpected return value from Manifests: Expected the return value to be `false` to indicate that it has NOT seen any change(s), but was `true`")

0 commit comments

Comments
 (0)