NE-2512: operator/ingress: Add dynamicConfigManager option#2757
NE-2512: operator/ingress: Add dynamicConfigManager option#2757Miciah wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@Miciah: This pull request references NE-2512 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-merge-robot
added
the
needs-rebase
|
Hello @Miciah! Some important instructions when contributing to openshift/api: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new public field 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Review Summary by QodoAdd dynamicConfigManager option to IngressController tuning configuration
WalkthroughsDescription• Add dynamicConfigManager field to IngressControllerTuningOptions • Enables OpenShift router's Dynamic Configuration Manager feature • Supports three values: empty string (default), "Enabled", "Disabled" • Includes comprehensive validation tests and API documentation Diagramflowchart LR
A["IngressControllerTuningOptions"] -->|"adds field"| B["dynamicConfigManager"]
B -->|"type"| C["IngressControllerDynamicConfigurationManager"]
C -->|"enum values"| D["Default/Enabled/Disabled"]
E["API Schema"] -->|"updated"| F["OpenAPI spec"]
G["Tests"] -->|"validates"| H["Creation and Update scenarios"]
File Changes1. operator/v1/types_ingress.go
|
Code Review by Qodo
1. Omitted dynamicConfigManager behavior undocumented
|
openshift-ci
bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml (1)
81-155: Consider adding additional state transition tests.The current onUpdate tests cover enabling from empty and switching from enabled to disabled. For more complete coverage, consider adding tests for:
- Transitioning from
DisabledtoEnabled- Resetting from
EnabledorDisabledback to""(default/no preference)These would ensure all valid state transitions are explicitly tested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml` around lines 81 - 155, The onUpdate tests only cover enabling from empty and enabled->disabled; add tests for the missing transitions by creating new onUpdate entries that use the same IngressController YAML structure and vary tuningOptions.dynamicConfigManager: add one test where initial is "Disabled" and updated is "Enabled" (name it e.g. "Should be able to turn DCM on when currently disabled") and add tests where initial is "Enabled" and updated is "" and where initial is "Disabled" and updated is "" (names like "Should be able to reset DCM to default from Enabled" and "Should be able to reset DCM to default from Disabled"); ensure expected blocks match the updated value and reuse the same fields (httpEmptyRequestsPolicy, idleConnectionTerminationPolicy, closedClientConnectionPolicy) so the harness picks up the transitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@operator/v1/types_ingress.go`:
- Around line 2091-2092: The json struct tag on the field
DynamicConfigurationManager is malformed (quote closes before omitempty) so it
serializes defaults instead of being omitted; update the tag for the
DynamicConfigurationManager field (type
IngressControllerDynamicConfigurationManager) to include omitempty inside the
quotes (e.g. json:"dynamicConfigManager,omitempty") so the field is omitted when
empty while keeping the existing feature-gate comment intact.
---
Nitpick comments:
In
`@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml`:
- Around line 81-155: The onUpdate tests only cover enabling from empty and
enabled->disabled; add tests for the missing transitions by creating new
onUpdate entries that use the same IngressController YAML structure and vary
tuningOptions.dynamicConfigManager: add one test where initial is "Disabled" and
updated is "Enabled" (name it e.g. "Should be able to turn DCM on when currently
disabled") and add tests where initial is "Enabled" and updated is "" and where
initial is "Disabled" and updated is "" (names like "Should be able to reset DCM
to default from Enabled" and "Should be able to reset DCM to default from
Disabled"); ensure expected blocks match the updated value and reuse the same
fields (httpEmptyRequestsPolicy, idleConnectionTerminationPolicy,
closedClientConnectionPolicy) so the harness picks up the transitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a7fa418-e5aa-498a-9c00-26363f5f6dc7
⛔ Files ignored due to path filters (7)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (4)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yamloperator/v1/types_ingress.gooperator/v1/zz_generated.featuregated-crd-manifests.yamloperator/v1/zz_generated.swagger_doc_generated.go
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
operator/v1/types_ingress.go
Outdated
| // dynamicConfigManager specifies whether OpenShift router should update | ||
| // HAProxy using its control socket for some configuration updates, | ||
| // instead of rewriting the HAProxy configuration file and instructing | ||
| // HAProxy to fork and reload. Enabling this option can improve | ||
| // load-balancing fairness and metrics accuracy and reduce CPU and | ||
| // memory usage if HAProxy has frequent configuration updates for route | ||
| // and endpoints updates. | ||
| // | ||
| // Note: This feature is currently experimental and should not be | ||
| // enabled on production clusters. | ||
| // | ||
| // The following values are valid for this field: | ||
| // | ||
| // * The empty string. | ||
| // * "Enabled". | ||
| // * "Disabled". | ||
| // | ||
| // The empty string specifies the default, which is "Disabled". Note | ||
| // that the default is subject to change. | ||
| // +openshift:enable:FeatureGate=IngressControllerDynamicConfigurationManager | ||
| DynamicConfigurationManager IngressControllerDynamicConfigurationManager `json:"dynamicConfigManager",omitempty"` |
There was a problem hiding this comment.
1. Omitted dynamicconfigmanager behavior undocumented 📘 Rule violation ✓ Correctness
The new DynamicConfigurationManager field comment does not explicitly describe what happens when the field is omitted, even though it is an optional API field. This violates the requirement to document omitted-field semantics for API fields so consumers understand defaulting behavior.
Agent Prompt
## Issue description
The `DynamicConfigurationManager` API field is optional, but its comment does not explicitly describe what happens when the field is omitted from the manifest. Compliance requires documenting omitted-field behavior for optional fields.
## Issue Context
The comment currently describes valid values and says the empty string is the default, but does not state whether omitting the field is equivalent to the empty string/default behavior.
## Fix Focus Areas
- operator/v1/types_ingress.go[2072-2092]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
api/operator/v1/types_ingress.go
Lines 2089 to 2090 in 03ec7f2
There was a problem hiding this comment.
Question:
api/operator/v1/types_ingress.go
Lines 2089 to 2090 in 03ec7f2
Answer:
This looks like a false-positive “documentation missing” finding: the field comment already states the omitted/empty semantics (your link to types_ingress.go#L2089-L2090).
To reduce repeats of this kind of issue, add a guardrail instruction to the improve tool so it verifies the existing comment text before flagging “omitted-field semantics undocumented”, and to explicitly treat “omitted” and “empty” wording as satisfying the requirement.
Add to your repo’s .pr_agent.toml (or wiki config):
[pr_code_suggestions]
extra_instructions = """\
Before raising any documentation/compliance issue about an optional API field (e.g., “omitted-field semantics undocumented”):
- Re-check the field’s existing comment text in the diff and treat explicit wording like “if omitted”, “when omitted”, “if empty”, “omitted/empty”, or “default is …” as sufficient omitted-field semantics.
- If such wording exists, do NOT report it as a rule violation; at most, suggest a minor clarity tweak (optional) and mark it non-blocking.
- Prefer linking/citing the exact lines that justify the conclusion (so reviewers can quickly verify).\
"""If you want to confirm the config is being applied on a PR run:
/improve --config.output_relevant_configurations=true
Relevant Sources:
- https://docs.qodo.ai/qodo-documentation/code-review/qodo-merge/tools/improve#extra-instructions-and-best-practices
- https://docs.qodo.ai/qodo-documentation/code-review/qodo-merge/usage-guide/additional_configurations#extra-instructions
- https://docs.qodo.ai/qodo-documentation/code-review/qodo-merge/usage-guide/additional_configurations#show-possible-configurations
operator/v1/types_ingress.go
Outdated
| // The empty string specifies the default, which is "Disabled". Note | ||
| // that the default is subject to change. | ||
| // +openshift:enable:FeatureGate=IngressControllerDynamicConfigurationManager | ||
| DynamicConfigurationManager IngressControllerDynamicConfigurationManager `json:"dynamicConfigManager",omitempty"` |
There was a problem hiding this comment.
2. Malformed json tag 🐞 Bug ✓ Correctness
IngressControllerTuningOptions.DynamicConfigurationManager uses a malformed struct tag (json:"dynamicConfigManager",omitempty"), so omitempty is not applied. This causes the generated OpenAPI to mark spec.tuningOptions.dynamicConfigManager as required, contradicting the API contract that tuningOptions fields are optional and potentially breaking OpenAPI-driven clients.
Agent Prompt
### Issue description
`IngressControllerTuningOptions.DynamicConfigurationManager` has a malformed JSON struct tag (`json:"dynamicConfigManager",omitempty"`), which prevents `omitempty` from being applied. This results in the generated OpenAPI schema incorrectly listing `dynamicConfigManager` as a required field, contradicting the API documentation that all `tuningOptions` fields are optional.
### Issue Context
This field is intended to be opt-in/optional (empty string means default), and OpenAPI consumers should not be forced to set it.
### Fix Focus Areas
- operator/v1/types_ingress.go[2072-2093]
- openapi/generated_openapi/zz_generated.openapi.go[54908-54918]
### Implementation notes
1. Change the tag to: `json:"dynamicConfigManager,omitempty"`.
2. Add `// +optional` on the field (consistent with other tuningOptions fields).
3. Re-run the repo’s generators (OpenAPI/swagger/CRDs) so the required-list entry for `dynamicConfigManager` disappears from generated OpenAPI.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Miciah
force-pushed
the
NE-2512-operator-slash-ingress-add-dynamicConfigManager-option
branch
from
ed58b15 to
03ec7f2
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
operator/v1/types_ingress.go (1)
2089-2092: Add the explicit optional marker for the new API field.This field is modeled as optional like the other tuning options, but it is the only new scalar here without
+optional. Adding the marker keeps the generated API metadata and docs aligned with the rest ofIngressControllerTuningOptions.Suggested diff
// The empty string specifies the default, which is "Disabled". Note // that the default is subject to change. + // +optional // +openshift:enable:FeatureGate=IngressControllerDynamicConfigurationManager DynamicConfigurationManager IngressControllerDynamicConfigurationManager `json:"dynamicConfigManager,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1/types_ingress.go` around lines 2089 - 2092, The new field DynamicConfigurationManager of type IngressControllerDynamicConfigurationManager is missing the +optional marker; add the comment marker "+optional" immediately above the DynamicConfigurationManager field declaration (matching the other tuning options in IngressControllerTuningOptions) so the generated API metadata/docs treat it as optional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml`:
- Around line 70-80: The expectedError string for the IngressController test
incorrectly lists the unsupported value sentinel as <nil>; update the assertion
to mention the empty-string sentinel "" instead. Locate the test block
referencing the IngressController resource (metadata name: default) and change
the expectedError text for spec.tuningOptions.dynamicConfigManager from '...,
<nil>' to '..., ""' so the message matches the validator output for an
empty-string allowed value.
---
Nitpick comments:
In `@operator/v1/types_ingress.go`:
- Around line 2089-2092: The new field DynamicConfigurationManager of type
IngressControllerDynamicConfigurationManager is missing the +optional marker;
add the comment marker "+optional" immediately above the
DynamicConfigurationManager field declaration (matching the other tuning options
in IngressControllerTuningOptions) so the generated API metadata/docs treat it
as optional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: badaf6d2-2222-4eab-806f-2603efbabe21
⛔ Files ignored due to path filters (9)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (2)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yamloperator/v1/types_ingress.go
...s/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml
Outdated
Show resolved
Hide resolved
Miciah
force-pushed
the
NE-2512-operator-slash-ingress-add-dynamicConfigManager-option
branch
from
03ec7f2 to
f123300
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml (1)
72-82:⚠️ Potential issue | 🟡 MinorFix the supported-values text in the exact-match assertion.
Line 82 still lists
<nil>as supported, but Lines 50-71 already establish""as the valid no-preference value. This exact-match assertion will fail even if validation is correct.✏️ Minimal fix
- expectedError: 'IngressController.operator.openshift.io "default" is invalid: spec.tuningOptions.dynamicConfigManager: Unsupported value: "garbage": supported values: "Enabled", "Disabled", <nil>' + expectedError: 'IngressController.operator.openshift.io "default" is invalid: spec.tuningOptions.dynamicConfigManager: Unsupported value: "garbage": supported values: "Enabled", "Disabled", ""'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml` around lines 72 - 82, Update the exact-match expectedError string for the test so the supported-values list matches the established no-preference value: replace the literal <nil> with the empty-string representation "" in the expectedError for spec.tuningOptions.dynamicConfigManager (the test that asserts Unsupported value: "garbage": supported values: "Enabled", "Disabled", <nil>) so it reads supported values: "Enabled", "Disabled", "".
🧹 Nitpick comments (1)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml (1)
50-71: Add a true omitted-field default-path case.This block covers the explicit
""case, but not the more common client behavior of omittingspec.tuningOptions.dynamicConfigManagerentirely. Since this field is meant to be opt-in, a create test for the omitted-field path would better protect the default-off contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml` around lines 50 - 71, Add a new test case named something like "Should be able to omit dynamicConfigManager to keep DCM off" that mirrors the existing explicit-empty-string case but omits the spec.tuningOptions.dynamicConfigManager field entirely; ensure the initial document does not include dynamicConfigManager and the expected document asserts the same defaults (httpEmptyRequestsPolicy, idleConnectionTerminationPolicy, closedClientConnectionPolicy) and that tuningOptions has no dynamicConfigManager entry, referencing the spec.tuningOptions.dynamicConfigManager path and the IngressController kind/name used in the other cases so the suite verifies the omitted-field (default-off) behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml`:
- Around line 72-82: Update the exact-match expectedError string for the test so
the supported-values list matches the established no-preference value: replace
the literal <nil> with the empty-string representation "" in the expectedError
for spec.tuningOptions.dynamicConfigManager (the test that asserts Unsupported
value: "garbage": supported values: "Enabled", "Disabled", <nil>) so it reads
supported values: "Enabled", "Disabled", "".
---
Nitpick comments:
In
`@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml`:
- Around line 50-71: Add a new test case named something like "Should be able to
omit dynamicConfigManager to keep DCM off" that mirrors the existing
explicit-empty-string case but omits the spec.tuningOptions.dynamicConfigManager
field entirely; ensure the initial document does not include
dynamicConfigManager and the expected document asserts the same defaults
(httpEmptyRequestsPolicy, idleConnectionTerminationPolicy,
closedClientConnectionPolicy) and that tuningOptions has no dynamicConfigManager
entry, referencing the spec.tuningOptions.dynamicConfigManager path and the
IngressController kind/name used in the other cases so the suite verifies the
omitted-field (default-off) behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ca9868ba-4f02-4159-8ed9-a417ae301da1
⛔ Files ignored due to path filters (9)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (2)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yamloperator/v1/types_ingress.go
🚧 Files skipped from review as they are similar to previous changes (1)
- operator/v1/types_ingress.go
Miciah
force-pushed
the
NE-2512-operator-slash-ingress-add-dynamicConfigManager-option
branch
from
f123300 to
6af006a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml (1)
72-82:⚠️ Potential issue | 🟡 MinorFix the supported-values text in the invalid-value assertion.
Line 82 still expects
<nil>, but this API now uses""as the empty/default sentinel. This exact-match assertion is likely to fail even when validation is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml` around lines 72 - 82, The test's expectedError string for the IngressController invalid-value assertion still expects `<nil>` but the API now uses an empty-string sentinel `""`; update the expectedError for the case named "Should not be able to specify a garbage value for DCM" to replace the trailing `<nil>` token with `""` (i.e., ensure spec.tuningOptions.dynamicConfigManager unsupported-values message lists "Enabled", "Disabled", ""), targeting the IngressController resource validation assertion in the test.
🧹 Nitpick comments (1)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml (1)
83-157: Add an update case that clears the field back to"".The matrix covers
"" -> EnabledandEnabled -> Disabled, but not reverting an explicit setting to the supported "no preference" state. That path is worth pinning down since the field docs explicitly allow""and note the default may change later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml` around lines 83 - 157, Add a new onUpdate test case to cover clearing dynamicConfigManager back to the empty string (""): create a case similar to the existing ones under onUpdate (use the pattern with name, initial, updated, expected) where initial has dynamicConfigManager set to Enabled or Disabled and updated sets dynamicConfigManager: "" and expected reflects the "" value; ensure the test case name clearly states it reverts to no-preference and place it alongside the existing cases to validate the Enabled/Disabled -> "" transition for the dynamicConfigManager field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml`:
- Around line 72-82: The test's expectedError string for the IngressController
invalid-value assertion still expects `<nil>` but the API now uses an
empty-string sentinel `""`; update the expectedError for the case named "Should
not be able to specify a garbage value for DCM" to replace the trailing `<nil>`
token with `""` (i.e., ensure spec.tuningOptions.dynamicConfigManager
unsupported-values message lists "Enabled", "Disabled", ""), targeting the
IngressController resource validation assertion in the test.
---
Nitpick comments:
In
`@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml`:
- Around line 83-157: Add a new onUpdate test case to cover clearing
dynamicConfigManager back to the empty string (""): create a case similar to the
existing ones under onUpdate (use the pattern with name, initial, updated,
expected) where initial has dynamicConfigManager set to Enabled or Disabled and
updated sets dynamicConfigManager: "" and expected reflects the "" value; ensure
the test case name clearly states it reverts to no-preference and place it
alongside the existing cases to validate the Enabled/Disabled -> "" transition
for the dynamicConfigManager field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9cf6e9da-fceb-4102-8db6-cf8424ec71e0
⛔ Files ignored due to path filters (9)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (2)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yamloperator/v1/types_ingress.go
Miciah
force-pushed
the
NE-2512-operator-slash-ingress-add-dynamicConfigManager-option
branch
from
6af006a to
3ccef73
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml (1)
85-159: Good transition coverage; consider extending for completeness.The onUpdate tests cover enabling from no-preference and disabling when enabled. For more comprehensive coverage, you might consider adding:
- Transition from
DisabledtoEnabled- Transition from
EnabledorDisabledback to""(no preference)- Invalid value rejection on update
These are optional since the current tests already demonstrate field mutability and the core transitions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml` around lines 85 - 159, Add more onUpdate test cases in the same onUpdate block to cover missing transitions: add a test named like "Should be able to enable DCM when currently Disabled" that sets initial tuningOptions.dynamicConfigManager: Disabled and updated: Enabled (expect Enabled); add a test named "Should accept clearing preference to empty string" with initial Enabled (and/or Disabled) and updated dynamicConfigManager: "" (expect ""), and add a negative test "Should reject invalid dynamicConfigManager value on update" where updated uses an invalid string and you assert the operator rejects it; place these cases alongside the existing scenarios in the onUpdate array so they exercise transitions Disabled->Enabled, Enabled/Disabled->"" and invalid-value handling for dynamicConfigManager.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml`:
- Around line 85-159: Add more onUpdate test cases in the same onUpdate block to
cover missing transitions: add a test named like "Should be able to enable DCM
when currently Disabled" that sets initial tuningOptions.dynamicConfigManager:
Disabled and updated: Enabled (expect Enabled); add a test named "Should accept
clearing preference to empty string" with initial Enabled (and/or Disabled) and
updated dynamicConfigManager: "" (expect ""), and add a negative test "Should
reject invalid dynamicConfigManager value on update" where updated uses an
invalid string and you assert the operator rejects it; place these cases
alongside the existing scenarios in the onUpdate array so they exercise
transitions Disabled->Enabled, Enabled/Disabled->"" and invalid-value handling
for dynamicConfigManager.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 600f505f-81b4-4eae-bd87-9c064cc0a6cf
⛔ Files ignored due to path filters (9)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (2)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yamloperator/v1/types_ingress.go
🚧 Files skipped from review as they are similar to previous changes (1)
- operator/v1/types_ingress.go
Miciah
force-pushed
the
NE-2512-operator-slash-ingress-add-dynamicConfigManager-option
branch
from
3ccef73 to
faf3394
Compare
This option enables OpenShift router's Dynamic Configuration Manager. This option is currently experimental, and it is not enabled by default; the default may change in later release. This commit implements NE-2512. https://issues.redhat.com/browse/NE-2512 * operator/v1/types_ingress.go (IngressControllerSpec): Add omitzero to the tuningOptions field to avoid 'Invalid value: "null"' errors in integration tests. (IngressControllerTuningOptions): Add MinProperties=1 so that integration tests pass. Add a new dynamicConfigManager field with type IngressControllerDynamicConfigurationManager. (IngressControllerDynamicConfigurationManager): New type. (IngressControllerDynamicConfigurationManagerDefault) (IngressControllerDynamicConfigurationManagerEnabled) (IngressControllerDynamicConfigurationManagerDisabled): New consts for the allowed values for the IngressControllerDynamicConfigurationManager type. * operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml: New file. Add tests for setting dynamicConfigManager to Enabled or Disabled on a new or updated ingresscontroller resource. * openapi/generated_openapi/zz_generated.openapi.go: * operator/v1/zz_generated.crd-manifests/*: * operator/v1/zz_generated.featuregated-crd-manifests.yaml: * operator/v1/zz_generated.featuregated-crd-manifests/*: * operator/v1/zz_generated.swagger_doc_generated.go: Regenerate.
Miciah
force-pushed
the
NE-2512-operator-slash-ingress-add-dynamicConfigManager-option
branch
from
faf3394 to
21dcb63
Compare
|
@Miciah: The following tests failed, say
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. |
| // The empty string specifies the default, which is "Disabled". Note | ||
| // that the default is subject to change. |
There was a problem hiding this comment.
We no longer recommend this kind of defaulting. Instead, we use omitted and the enum should not allow the "" value.
For "" to be valid, you need it be able to be round tripped by a structured client, so it either needs to be a pointer, or, drop the omitempty. Dropping omitempty affects HyperShift, because we then marshal an empty string when we upgrade libraries, where we previously didn't. Adding a pointer would work, but then why have a "" value at all?
We used to do this for discoverability but it's caused too many issues and we no longer recommend it
There was a problem hiding this comment.
All right, I'll replace the empty string with a "NoOpinion" value.
| IngressControllerDynamicConfigurationManagerEnabled IngressControllerDynamicConfigurationManager = "Enabled" | ||
| IngressControllerDynamicConfigurationManagerDisabled IngressControllerDynamicConfigurationManager = "Disabled" |
There was a problem hiding this comment.
What if the values actually meant something about the behaviour, rather than being proxies for true and false?
configurationManagement: Dynamic | ForkAndReload
There was a problem hiding this comment.
Which is better: configurationManagement or configManagement? It seems that we usually abbreviate configuration in API fields.
Given that the Dynamic Configuration Manager in fact falls back to fork-and-reload when it cannot perform the update itself, is providing these two options likely to cause confusion?
There was a problem hiding this comment.
I don't mind configuration vs config, we do normally contract it as you say
Given that the Dynamic Configuration Manager in fact falls back to fork-and-reload when it cannot perform the update itself, is providing these two options likely to cause confusion?
This detail is not currently in the godoc (or at least I missed it), I'm open to suggestions for good names for these, my point was more, we can probably be descriptive about the values and provide something more meaningful than Enabled/Disabled and then in the future if a third method came along, we could support that in the same enum
| // | ||
| // +optional | ||
| TuningOptions IngressControllerTuningOptions `json:"tuningOptions,omitempty"` | ||
| TuningOptions IngressControllerTuningOptions `json:"tuningOptions,omitempty,omitzero"` |
There was a problem hiding this comment.
Omitempty is pointless here, you can remove it
There was a problem hiding this comment.
I'll drop it in the next push. make update takes 15-30 minutes, so I'll pick this up tomorrow.
There was a problem hiding this comment.
Eek, it shouldn't be taking that long.
You don't need proto so try PROTO_OPTIONAL=1 make update. Takes less than 5m on my machine 🤷
| // IngressControllerTuningOptions specifies options for tuning the performance | ||
| // of ingress controller pods | ||
| // | ||
| // +kubebuilder:validation:MinProperties=1 |
There was a problem hiding this comment.
This could be a breaking change. Since we didn't previously have omitzero on this struct, any structured client who wrote out spec would have marshalled tuningOptions: {}
This is a positive change in general, but before we commit to it, we will need to test if this ratchets
There was a problem hiding this comment.
I might as well drop it if the new field cannot have a default empty value.
There was a problem hiding this comment.
All fields in this struct already have omitempty, so discoverability was never a thing on this struct anyway, it's kind of an odd mix 🤔
4 participants
This option enables OpenShift router's Dynamic Configuration Manager. This option is currently experimental, and it is not enabled by default; however, the default setting may change in later release.
We recently decided that we can never graduate this feature to GA or enable it by default without having an option to turn it off, and so I am adding an opt-in API field now while the feature is tech preview.
Cc: @jcmoraisjr