-
Notifications
You must be signed in to change notification settings - Fork 157
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
Implement calculate diffs and rendering diffs of k8s #5674
Conversation
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5674 +/- ##
==========================================
+ Coverage 25.60% 25.75% +0.15%
==========================================
Files 478 478
Lines 51307 51400 +93
==========================================
+ Hits 13135 13236 +101
+ Misses 37167 37160 -7
+ Partials 1005 1004 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
return len(r.Adds) + len(r.Deletes) + len(r.Changes) | ||
} | ||
|
||
func DiffList(liveManifests, desiredManifests []Manifest, logger *zap.Logger, opts ...diff.Option) (*DiffListResult, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref;
pipecd/pkg/app/piped/platformprovider/kubernetes/diff.go
Lines 79 to 103 in 20577c0
func DiffList(olds, news []Manifest, logger *zap.Logger, opts ...diff.Option) (*DiffListResult, error) { | |
adds, deletes, newChanges, oldChanges := groupManifests(olds, news) | |
cr := &DiffListResult{ | |
Adds: adds, | |
Deletes: deletes, | |
Changes: make([]DiffListChange, 0, len(newChanges)), | |
} | |
for i := 0; i < len(newChanges); i++ { | |
result, err := Diff(oldChanges[i], newChanges[i], logger, opts...) | |
if err != nil { | |
return nil, err | |
} | |
if !result.HasDiff() { | |
continue | |
} | |
cr.Changes = append(cr.Changes, DiffListChange{ | |
Old: oldChanges[i], | |
New: newChanges[i], | |
Diff: result, | |
}) | |
} | |
return cr, nil | |
} |
MaxChangedManifests int | ||
} | ||
|
||
func (r *DiffListResult) Render(opt DiffRenderOptions) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref;
pipecd/pkg/app/piped/platformprovider/kubernetes/diff.go
Lines 148 to 212 in 20577c0
func (r *DiffListResult) Render(opt DiffRenderOptions) string { | |
var b strings.Builder | |
index := 0 | |
for _, delete := range r.Deletes { | |
index++ | |
b.WriteString(fmt.Sprintf("- %d. %s\n\n", index, delete.Key.ReadableString())) | |
} | |
for _, add := range r.Adds { | |
index++ | |
b.WriteString(fmt.Sprintf("+ %d. %s\n\n", index, add.Key.ReadableString())) | |
} | |
maxPrintDiffs := len(r.Changes) | |
if opt.MaxChangedManifests != 0 && opt.MaxChangedManifests < maxPrintDiffs { | |
maxPrintDiffs = opt.MaxChangedManifests | |
} | |
var prints = 0 | |
for _, change := range r.Changes { | |
key := change.Old.Key | |
opts := []diff.RenderOption{ | |
diff.WithLeftPadding(1), | |
} | |
needMaskValue := false | |
if opt.MaskSecret && key.IsSecret() { | |
opts = append(opts, diff.WithMaskPath("data")) | |
needMaskValue = true | |
} else if opt.MaskConfigMap && key.IsConfigMap() { | |
opts = append(opts, diff.WithMaskPath("data")) | |
needMaskValue = true | |
} | |
renderer := diff.NewRenderer(opts...) | |
index++ | |
b.WriteString(fmt.Sprintf("# %d. %s\n\n", index, key.ReadableString())) | |
// Use our diff check in one of the following cases: | |
// - not explicit set useDiffCommand option. | |
// - requires masking secret or configmap value. | |
if !opt.UseDiffCommand || needMaskValue { | |
b.WriteString(renderer.Render(change.Diff.Nodes())) | |
} else { | |
// TODO: Find a way to mask values in case of using unix `diff` command. | |
d, err := diffByCommand(diffCommand, change.Old, change.New) | |
if err != nil { | |
b.WriteString(fmt.Sprintf("An error occurred while rendering diff (%v)", err)) | |
} else { | |
b.Write(d) | |
} | |
} | |
b.WriteString("\n") | |
prints++ | |
if prints >= maxPrintDiffs { | |
break | |
} | |
} | |
if prints < len(r.Changes) { | |
b.WriteString(fmt.Sprintf("... (omitted %d other changed manifests\n", len(r.Changes)-prints)) | |
} | |
return b.String() | |
} |
I omit the option to use diff command to calculate diff.
return result, nil | ||
} | ||
|
||
func groupManifests(olds, news []Manifest) (adds, deletes, newChanges, oldChanges []Manifest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref;
pipecd/pkg/app/piped/platformprovider/kubernetes/diff.go
Lines 266 to 305 in 20577c0
func groupManifests(olds, news []Manifest) (adds, deletes, newChanges, oldChanges []Manifest) { | |
// Sort the manifests before comparing. | |
sort.Slice(news, func(i, j int) bool { | |
return news[i].Key.IsLessWithIgnoringNamespace(news[j].Key) | |
}) | |
sort.Slice(olds, func(i, j int) bool { | |
return olds[i].Key.IsLessWithIgnoringNamespace(olds[j].Key) | |
}) | |
var n, o int | |
for { | |
if n >= len(news) || o >= len(olds) { | |
break | |
} | |
if news[n].Key.IsEqualWithIgnoringNamespace(olds[o].Key) { | |
newChanges = append(newChanges, news[n]) | |
oldChanges = append(oldChanges, olds[o]) | |
n++ | |
o++ | |
continue | |
} | |
// Has in news but not in olds so this should be a added one. | |
if news[n].Key.IsLessWithIgnoringNamespace(olds[o].Key) { | |
adds = append(adds, news[n]) | |
n++ | |
continue | |
} | |
// Has in olds but not in news so this should be an deleted one. | |
deletes = append(deletes, olds[o]) | |
o++ | |
} | |
if len(news) > n { | |
adds = append(adds, news[n:]...) | |
} | |
if len(olds) > o { | |
deletes = append(deletes, olds[o:]...) | |
} | |
return | |
} |
I changed it to compare with the namespace because the plugin's implementation sets the namespace when loading and parsing manifests, so I think it's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you go 👍
What this PR does:
as title
Why we need it:
We have to calculate diffs to implement DriftDetection
Which issue(s) this PR fixes:
Part of #5363
Does this PR introduce a user-facing change?: No