[SREP-3287] dynatrace_enabled feature flag#466
Open
aliceh wants to merge 4 commits intoopenshift:masterfrom
Open
[SREP-3287] dynatrace_enabled feature flag#466aliceh wants to merge 4 commits intoopenshift:masterfrom
aliceh wants to merge 4 commits intoopenshift:masterfrom
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aliceh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #466 +/- ##
==========================================
- Coverage 55.86% 55.79% -0.08%
==========================================
Files 31 31
Lines 2753 2762 +9
==========================================
+ Hits 1538 1541 +3
- Misses 1123 1129 +6
Partials 92 92
🚀 New features to boost your workflow:
|
aliceh
force-pushed
the
SREP-3287-dev
branch
from
547b058 to
b8ceddf
Compare
- Resolved merge conflict in rmo-config-template.yaml - Kept both dynatrace-enabled config field and new monitoring resources - All hostedcontrolplane tests passing Made-with: Cursor
WalkthroughIntroduces Dynatrace synthetic monitoring integration for route-monitor-operator via a feature flag (dynatrace-enabled) configurable through ConfigMap. Updates configuration reading to parse the flag, conditionally instantiates Dynatrace API clients, and modifies reconciliation logic to handle Dynatrace resources only when enabled. Changes
Sequence DiagramsequenceDiagram
participant R as HostedControlPlane<br/>Reconciler
participant C as ConfigMap<br/>(rmo-config)
participant D as Dynatrace<br/>API Client
participant RO as RHOBS<br/>Monitoring
R->>C: Read configuration from ConfigMap
C-->>R: Return RHOBS & Dynatrace configs
alt Dynatrace Enabled
R->>D: Create Dynatrace API client
alt Client Creation Success
D-->>R: Client instance
R->>R: Store dynatraceApiClient
Note over R: Both RHOBS and Dynatrace<br/>monitoring active
else Client Creation Fails
D-->>R: Error
alt RHOBS Configured
R->>RO: Continue with RHOBS only
RO-->>R: Proceed with fallback
else RHOBS Not Configured
R->>R: Return fatal error
end
end
else Dynatrace Disabled
R->>R: Log: Dynatrace monitoring disabled
R->>RO: Use RHOBS monitoring only
end
R->>R: Reconcile resources
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
controllers/hostedcontrolplane/hostedcontrolplane_test.go (1)
1041-1044: Avoid mutating expected values by test-case name.This introduces hidden coupling and makes failures harder to reason about. Keep each case self-contained in the table instead.
♻️ Proposed cleanup
- expectedRHOBS: RHOBSConfig{ - ProbeAPIURL: "fallback-api.example.com/probes", // wrong, should be fallback + expectedRHOBS: RHOBSConfig{ + ProbeAPIURL: "https://fallback-api.example.com/probes", Tenant: "fallback-tenant", // empty/whitespace uses fallback OIDCClientID: "valid-id", OIDCClientSecret: "fallback-secret", OIDCIssuerURL: "https://fallback-issuer.example.com", OnlyPublicClusters: false, }, @@ - // For the empty values test, we need to fix the expected value - if tt.name == "ConfigMap present with empty values - uses fallback for empty fields" { - tt.expectedRHOBS.ProbeAPIURL = "https://fallback-api.example.com/probes" - } - if rhobsResult != tt.expectedRHOBS { t.Errorf("getRHOBSConfig() RHOBS got = %+v, want = %+v", rhobsResult, tt.expectedRHOBS) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/hostedcontrolplane/hostedcontrolplane_test.go` around lines 1041 - 1044, The test mutates the expected result based on tt.name (the conditional that sets tt.expectedRHOBS.ProbeAPIURL when tt.name == "ConfigMap present with empty values - uses fallback for empty fields"), which creates hidden coupling; remove that conditional and instead set the correct ProbeAPIURL directly in the test-case table entry for that case (populate expectedRHOBS.ProbeAPIURL = "https://fallback-api.example.com/probes" in the case struct), ensuring each case is self-contained and keeping tt, tt.name and tt.expectedRHOBS.ProbeAPIURL unchanged elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/hostedcontrolplane/hostedcontrolplane.go`:
- Line 114: The current return of DynatraceConfig{Enabled: false} in the hosted
control plane config code inappropriately disables Dynatrace when the ConfigMap
is unreadable/missing/invalid; change the default to DynatraceConfig{Enabled:
true} unless the ConfigMap explicitly sets the value to the string "false".
Update the code paths that currently return r.RHOBSConfig,
DynatraceConfig{Enabled: false} (and the analogous block around the 138-142
range) to return r.RHOBSConfig, DynatraceConfig{Enabled: true} by default, and
add explicit handling to parse the ConfigMap value into DynatraceConfig.Enabled
so only an explicit "false" disables it; keep references to RHOBSConfig and
DynatraceConfig to locate the changes.
In `@hack/olm-registry/rmo-config-template.yaml`:
- Around line 50-53: The template currently sets the DYNATRACE_ENABLED parameter
default to "false", which makes generated ConfigMaps opt-out; change the default
value for the parameter named DYNATRACE_ENABLED in rmo-config-template.yaml from
"false" to "true" (and keep required: false) so Dynatrace is enabled by default,
and update the description if needed to state that it is enabled by default and
must be explicitly set to "false" to disable.
In `@README.md`:
- Around line 67-68: The README currently says `dynatrace-enabled` defaults to
"false" but the PR requires Dynatrace to be enabled by default; update the field
description for `dynatrace-enabled` to indicate default: "true" and adjust any
examples/usage blocks (referenced around the same section lines and the example
at lines 73-78) so they reflect that Dynatrace is enabled unless explicitly set
to "false" (change wording and example values accordingly).
---
Nitpick comments:
In `@controllers/hostedcontrolplane/hostedcontrolplane_test.go`:
- Around line 1041-1044: The test mutates the expected result based on tt.name
(the conditional that sets tt.expectedRHOBS.ProbeAPIURL when tt.name ==
"ConfigMap present with empty values - uses fallback for empty fields"), which
creates hidden coupling; remove that conditional and instead set the correct
ProbeAPIURL directly in the test-case table entry for that case (populate
expectedRHOBS.ProbeAPIURL = "https://fallback-api.example.com/probes" in the
case struct), ensuring each case is self-contained and keeping tt, tt.name and
tt.expectedRHOBS.ProbeAPIURL unchanged elsewhere.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
README.mdcontrollers/hostedcontrolplane/hostedcontrolplane.gocontrollers/hostedcontrolplane/hostedcontrolplane_test.gocontrollers/hostedcontrolplane/synthetics.gohack/olm-registry/rmo-config-template.yaml
| logger.V(2).Info("Failed to read ConfigMap, using fallback config", "error", err.Error()) | ||
| } | ||
| return r.RHOBSConfig | ||
| return r.RHOBSConfig, DynatraceConfig{Enabled: false} |
There was a problem hiding this comment.
Dynatrace failover/default logic is reversed from the feature contract.
Current logic disables Dynatrace when the ConfigMap is unreadable, missing, empty, or invalid. Per PR objective, Dynatrace should be enabled unless explicitly set to "false".
🐛 Proposed fix
- return r.RHOBSConfig, DynatraceConfig{Enabled: false}
+ return r.RHOBSConfig, DynatraceConfig{Enabled: true}
@@
- // Read Dynatrace configuration - defaults to disabled
- dynatraceConfig := DynatraceConfig{Enabled: false}
- if strings.TrimSpace(configMap.Data["dynatrace-enabled"]) == "true" {
- dynatraceConfig.Enabled = true
- }
+ // Read Dynatrace configuration - defaults to enabled unless explicitly disabled
+ dynatraceConfig := DynatraceConfig{Enabled: true}
+ if strings.TrimSpace(configMap.Data["dynatrace-enabled"]) == "false" {
+ dynatraceConfig.Enabled = false
+ }Also applies to: 138-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controllers/hostedcontrolplane/hostedcontrolplane.go` at line 114, The
current return of DynatraceConfig{Enabled: false} in the hosted control plane
config code inappropriately disables Dynatrace when the ConfigMap is
unreadable/missing/invalid; change the default to DynatraceConfig{Enabled: true}
unless the ConfigMap explicitly sets the value to the string "false". Update the
code paths that currently return r.RHOBSConfig, DynatraceConfig{Enabled: false}
(and the analogous block around the 138-142 range) to return r.RHOBSConfig,
DynatraceConfig{Enabled: true} by default, and add explicit handling to parse
the ConfigMap value into DynatraceConfig.Enabled so only an explicit "false"
disables it; keep references to RHOBSConfig and DynatraceConfig to locate the
changes.
Comment on lines
+67
to
+68
| - `dynatrace-enabled`: Enable/disable Dynatrace synthetic monitoring (default: "false") | ||
|
|
There was a problem hiding this comment.
Dynatrace default behavior in docs is inverted.
This section documents Dynatrace as disabled by default (dynatrace-enabled default "false"), but the PR objective states Dynatrace should be enabled unless explicitly set to "false". Please align the field description and example accordingly.
Also applies to: 73-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 67 - 68, The README currently says
`dynatrace-enabled` defaults to "false" but the PR requires Dynatrace to be
enabled by default; update the field description for `dynatrace-enabled` to
indicate default: "true" and adjust any examples/usage blocks (referenced around
the same section lines and the example at lines 73-78) so they reflect that
Dynatrace is enabled unless explicitly set to "false" (change wording and
example values accordingly).
Contributor
|
@aliceh: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation