-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
feat: Introduce diff normalizer knobs and allow for ignoring aggregated cluster roles (#2382) #3076
feat: Introduce diff normalizer knobs and allow for ignoring aggregated cluster roles (#2382) #3076
Conversation
compareOptions, err := m.settingsMgr.GetResourceCompareOptions() | ||
if err != nil { | ||
log.Warnf("Could not get compare options from ConfigMap (assuming defaults): %v", err) | ||
compareOptions = diff.GetDefaultDiffOptions() |
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.
Since CompareAppState
does not return faulty (no error
return value), I deliberately chose to use default compare options if we can not get the options from the settings manager.
Codecov Report
@@ Coverage Diff @@
## master #3076 +/- ##
==========================================
- Coverage 44.47% 39.09% -5.39%
==========================================
Files 184 49 -135
Lines 21006 1407 -19599
Branches 275 238 -37
==========================================
- Hits 9342 550 -8792
+ Misses 10625 854 -9771
+ Partials 1039 3 -1036 Continue to review full report at Codecov.
|
@jessesuen This is the second attempt to introduce #2382, and I followed your (absolutely correct) suggestions from previous PR #2422 (which probably was way overengineered). I think the current approach is better and more simple. @alexmt I understand that this change interferes a little with changes in #3066 - it's up to you wether we merge this one first and adapt changes in #3066, or merge #3066 first and then adapt changes here for further merging. |
…t/2382-aggr-cluster-roles2
…t/2382-aggr-cluster-roles2
90b950f
to
a5d17ab
Compare
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
…t/2382-aggr-cluster-roles2
This PR introduces options for the diff normalizer to allow tuning its behaviour for well-known K8s behaviours.
The first use case is to ignore the rules in aggregated cluster roles, which are dynamically determined by K8s at runtime and would be out-of-sync always. This will fix #2382
A new setting
resource.compareoptions
is introduced inargocd-cm
, which hold knobs to control behaviour of the diff normalizer.By setting
ignoreAggregratedRoles
inresource.compareoptions
totrue
, the rules will not be considered for determining the sync state of the role.The default value of
ignoreAggregratedRoles
will befalse
to not change behaviour from previous versions of ArgoCD.The new knob can be set like follows in
argocd-cm
:It's only valid system wide, not on a per-application basis.
Checklist: