Skip to content

Conversation

@yozel
Copy link
Contributor

@yozel yozel commented Dec 4, 2025

This PR Introduces a new condition type "Drifted" to improve observability of Helm release drift detection.

Fix: #1241

@yozel yozel changed the title Add Drifted condition Add Drifted condition to HelmRelease and log generation Dec 4, 2025
@yozel yozel force-pushed the yozel/add-drifted-condition branch 3 times, most recently from 7f9cf56 to e10328c Compare December 9, 2025 13:01
@yozel yozel force-pushed the yozel/add-drifted-condition branch from e10328c to 238ebbe Compare December 17, 2025 10:19
@yozel yozel changed the title Add Drifted condition to HelmRelease and log generation Add Drifted condition to HelmRelease Dec 24, 2025
@yozel yozel force-pushed the yozel/add-drifted-condition branch from 238ebbe to 627159a Compare January 8, 2026 11:59
@yozel yozel force-pushed the yozel/add-drifted-condition branch from 627159a to c7d9ebf Compare January 8, 2026 14:52
@matheuscscp
Copy link
Member

It would be nice to add a test for when the condition is false

@yozel yozel force-pushed the yozel/add-drifted-condition branch from c7d9ebf to fb7ed67 Compare January 8, 2026 15:33
@yozel
Copy link
Contributor Author

yozel commented Jan 8, 2026

It would be nice to add a test for when the condition is false

I'll add it right now

@yozel yozel force-pushed the yozel/add-drifted-condition branch from fb7ed67 to 0d71cdf Compare January 9, 2026 15:01
Copy link
Member

@matheuscscp matheuscscp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

Comment on lines +388 to +396
if req.Object.GetDriftDetection().MustDetectChanges() {
conditions.MarkFalse(req.Object, v2.DriftedCondition, v2.NoDriftDetectedReason, "No drift detected against the cluster state")
}

Copy link
Member

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?

Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@yozel yozel force-pushed the yozel/add-drifted-condition branch 4 times, most recently from 5246c26 to 564292c Compare January 9, 2026 22:16
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>
@yozel yozel force-pushed the yozel/add-drifted-condition branch from 564292c to 5e52cb3 Compare January 9, 2026 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Drift detection in warn mode - having status field or metrics to have alerting

3 participants