-
Notifications
You must be signed in to change notification settings - Fork 198
Add Drifted condition to HelmRelease
#1367
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
base: main
Are you sure you want to change the base?
Conversation
Drifted conditionDrifted condition to HelmRelease and log generation
7f9cf56 to
e10328c
Compare
e10328c to
238ebbe
Compare
Drifted condition to HelmRelease and log generationDrifted condition to HelmRelease
238ebbe to
627159a
Compare
627159a to
c7d9ebf
Compare
|
It would be nice to add a test for when the condition is false |
c7d9ebf to
fb7ed67
Compare
I'll add it right now |
fb7ed67 to
0d71cdf
Compare
matheuscscp
left a comment
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! 🚀
| if req.Object.GetDriftDetection().MustDetectChanges() { | ||
| conditions.MarkFalse(req.Object, v2.DriftedCondition, v2.NoDriftDetectedReason, "No drift detected against the cluster state") | ||
| } | ||
|
|
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.
Shouldn't we remove this condition if drift is not enabled in spec?
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.
Maybe put an else to remove the condition if MustDetectChanges() returns false?
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.
@stefanprodan good point.
@matheuscscp then it would not be removed in case release state is not InSync. I can remove the condition in the beginning of AtomicRelease.actionForState here or even in AtomicRelease.Reconcile here, so we can be sure the condition is removed in case drift detection disabled later on
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.
Works for me
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.
Done
5246c26 to
564292c
Compare
This commit introduces a new Kubernetes condition type "Drifted" to improve observability of Helm release drift detection. Signed-off-by: Yasin Özel <yozel@nebius.com>
564292c to
5e52cb3
Compare
This PR Introduces a new condition type "Drifted" to improve observability of Helm release drift detection.
Fix: #1241