NE-2777, NE-2778: Implement Gateway API management knob#2890
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@rikatz: This pull request references NE-2777 which is a valid jira issue. This pull request references NE-2778 which is a valid jira issue. 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. |
|
Hello @rikatz! Some important instructions when contributing to openshift/api: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR introduces the Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented 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 |
openshift-ci
Bot
added
the
size/XL
|
[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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator/v1alpha1/types_ingress.go`:
- Around line 28-30: The Spec field at lines 28-30 and the gatewayAPI field at
lines 43-49 both have +required markers but their field comments do not
explicitly document the requiredness semantics. Update the comment for the Spec
field to explicitly state that it is required, and similarly update the comment
for the gatewayAPI field to explicitly describe its required nature, ensuring
that each marker has corresponding documentation that clearly describes the
requiredness or omitted behavior per the API marker-documentation checklist and
coding guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c8b584c2-220b-4f2c-b665-66a4d43f5e44
⛔ Files ignored due to path filters (6)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1alpha1/zz_generated.crd-manifests/0000_50_ingress_02_ingresses.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*operator/v1alpha1/zz_generated.featuregated-crd-manifests/ingresses.operator.openshift.io/GatewayAPIManagementMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (11)
features.mdfeatures/features.gooperator/v1alpha1/types_ingress.gopayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
rikatz
force-pushed
the
ingress-config-operator
branch
2 times, most recently
from
b4fe018 to
aa6d998
Compare
rikatz
force-pushed
the
ingress-config-operator
branch
from
aa6d998 to
652792d
Compare
|
@rikatz: The following test 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. |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Overall, this looks pretty good. A handful of comments.
| contactPerson("miciah"). | ||
| productScope(ocpSpecific). | ||
| enhancementPR("https://github.com/openshift/enhancements/pull/2023"). | ||
| enable(inTechPreviewNoUpgrade(), inDevPreviewNoUpgrade()). |
There was a problem hiding this comment.
Just a note that we are now enforcing that before a feature gate can be included in the TPNU feature set that the enhancement must be merged.
Please get the EP merged before we merge this - thanks!
| // configuration, which sets managementMode to "Managed". | ||
| // | ||
| // +optional | ||
| // +openshift:enable:FeatureGate=GatewayAPIManagementMode |
There was a problem hiding this comment.
The whole API is gated, so we can drop this marker.
| // GatewayClass, Gateway resources). This is the only fully | ||
| // supported configuration. |
There was a problem hiding this comment.
Both the current options seem like they would be considered "fully supported", so the final sentence here might be a bit misleading.
| // managementMode specifies how the Cluster Ingress | ||
| // Operator manages Gateway API Custom Resource Definitions | ||
| // (CRDs) and the associated Gateway controller stack. | ||
| // |
There was a problem hiding this comment.
For enum fields, we generally encourage a sentence here that explicitly states what the set of allowed values are like:
Allowed values are Managed and Unmanaged.
| // When omitted, the field defaults to "Managed". | ||
| // | ||
| // +default="Managed" | ||
| // +optional |
There was a problem hiding this comment.
I don't think this default marker does anything if the parent field isn't also specified, only if this field is explicitly omitted.
The minimum properties on this type inherently makes this field required when specifying the parent field.
I'm wondering, should this be a required field regardless? I'm not sure it makes sense to explicitly specify the Gateway API configuration field without explicitly providing the management mode you'd like to be in place.
4 participants
This PR implements the feature gate and the API required for the implementation of Gateway APi management knob on OCP.
EP: openshift/enhancements#2023