-
Notifications
You must be signed in to change notification settings - Fork 129
Levan m/dca ccr reconciler refactor #2380
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2380 +/- ##
==========================================
+ Coverage 36.97% 37.33% +0.36%
==========================================
Files 287 287
Lines 24257 24664 +407
==========================================
+ Hits 8968 9208 +240
- Misses 14590 14750 +160
- Partials 699 706 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| existingDeployment := &appsv1.Deployment{} | ||
| if err := r.client.Get(ctx, nsName, existingDeployment); err != nil { | ||
| if errors.IsNotFound(err) { | ||
| deleteStatusWithClusterChecksRunner(newStatus, common.ClusterChecksRunnerReconcileConditionType, setClusterChecksRunnerStatus) |
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.
| deleteStatusWithClusterChecksRunner(newStatus, common.ClusterChecksRunnerReconcileConditionType, setClusterChecksRunnerStatus) | |
| deleteStatusV2WithClusterAgent(newStatus, common.ClusterAgentReconcileConditionType, setClusterAgentStatus) |
This should be DCA I believe ? Not sure how we can enter this condition tho to verify
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.
Correct thanks for catching updated 77e27a5.
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 believe it should also be common.ClusterAgentReconcileConditionType as the argument
| // getDeploymentNameFromCCR returns the expected CCR deployment name based on | ||
| // the DDA name and clusterChecksRunner name override | ||
| func getDeploymentNameFromCCR(dda *datadoghqv2alpha1.DatadogAgent) string { | ||
| deploymentName := clusterchecksrunner.GetClusterChecksRunnerName(dda) | ||
| if componentOverride, ok := dda.Spec.Override[datadoghqv2alpha1.ClusterChecksRunnerComponentName]; ok { | ||
| if componentOverride.Name != nil && *componentOverride.Name != "" { | ||
| deploymentName = *componentOverride.Name | ||
| } | ||
| } | ||
| return deploymentName | ||
| } |
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.
@ Timothée: common.GetDeploymentNameFromDatadogAgent gets DCA and does the same thing as this for CCR, only change is component + GetClusterAgentName vs GetClusterChecksRunnerName -> getComponentDeployNameFromDDA(ddaObject, ddaSpec, componentName) + getComponentName(dda, component)
What does this PR do?
cleanupOldCCR**function to file where it's used. Changed some log lines, variable names.Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Minimum Agent Versions
Are there minimum versions of the Datadog Agent and/or Cluster Agent required?
Describe your test plan
Write there any instructions and details you may have to test your PR.
Checklist
bug,enhancement,refactoring,documentation,tooling, and/ordependenciesqa/skip-qalabel