-
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 the sync state calculation for the kubernetes livestate plugin #5676
Conversation
…gin. Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
… plugin Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5676 +/- ##
==========================================
- Coverage 25.73% 25.68% -0.06%
==========================================
Files 478 476 -2
Lines 51400 50973 -427
==========================================
- Hits 13230 13092 -138
+ Misses 37166 36890 -276
+ Partials 1004 991 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
}, nil | ||
} | ||
|
||
func calculateSyncState(diffResult *provider.DiffListResult, commit string) sdk.ApplicationSyncState { |
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.
reference implementation is here
pipecd/pkg/app/piped/driftdetector/kubernetes/detector.go
Lines 385 to 422 in 57c48c1
func makeSyncState(r *provider.DiffListResult, commit string) model.ApplicationSyncState { | |
if r.NoChange() { | |
return model.ApplicationSyncState{ | |
Status: model.ApplicationSyncStatus_SYNCED, | |
ShortReason: "", | |
Reason: "", | |
Timestamp: time.Now().Unix(), | |
} | |
} | |
total := len(r.Adds) + len(r.Deletes) + len(r.Changes) | |
shortReason := fmt.Sprintf("There are %d manifests not synced (%d adds, %d deletes, %d changes)", total, len(r.Adds), len(r.Deletes), len(r.Changes)) | |
if len(commit) >= 7 { | |
commit = commit[:7] | |
} | |
var b strings.Builder | |
b.WriteString(fmt.Sprintf("Diff between the defined state in Git at commit %s and actual state in cluster:\n\n", commit)) | |
b.WriteString("--- Actual (LiveState)\n+++ Expected (Git)\n\n") | |
details := r.Render(provider.DiffRenderOptions{ | |
MaskSecret: true, | |
MaskConfigMap: true, | |
MaxChangedManifests: 3, | |
// Currently, we do not use the diff command to render the result | |
// because Kubernetes adds a large number of default values to the | |
// running manifest that causes a wrong diff text. | |
UseDiffCommand: false, | |
}) | |
b.WriteString(details) | |
return model.ApplicationSyncState{ | |
Status: model.ApplicationSyncStatus_OUT_OF_SYNC, | |
ShortReason: shortReason, | |
Reason: b.String(), | |
Timestamp: time.Now().Unix(), | |
} | |
} |
}, | ||
}, | ||
{ | ||
name: "service and ingress changes", |
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.
[ask] Does the logic for calculateSyncState
depend on each resource kind?
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.
The ConfigMap and Secrets are masked. The other kinds are treated as the same.
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.
I see. Then, we can fix the test case name to clarify the spec for the function.
For example "changed two resources". WDYT?
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.
OK, it's a good name to show the spec.
Also, I'll change the second test case, deployment spec changes
, to changed one resource
.
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.
Fixed on this commit
9a4b84a
} | ||
} | ||
|
||
func TestCalculateSyncState(t *testing.T) { |
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.
[nit] It would be nice to add some testcases to cover the spec.
- The maximum count of the changes is three.
- ConfigMap and Secret are masked on the text
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.
Thank you!
Added at this commit 2306bc9
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
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.
Sorry, just only one more nit 🙏
Co-authored-by: Yoshiki Fujikane <[email protected]> Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
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.
LGTM
What this PR does:
as title
Why we need it:
We want to support DriftDetection feature with k8s plugin
Which issue(s) this PR fixes:
Part of #5363
Does this PR introduce a user-facing change?: No