ACM-32735: Add better API docs to CRDs#10124
ACM-32735: Add better API docs to CRDs#10124carbonin wants to merge 1 commit intoopenshift:masterfrom
Conversation
Update CRDs with more useful API documentation. This focuses on the fields called out in ACM-32735 specifically. Resolves https://redhat.atlassian.net/browse/ACM-32735
openshift-ci-robot
added
the
jira/valid-reference
|
@carbonin: This pull request references ACM-32735 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-ci
bot
added
the
size/XL
openshift-ci
bot
added
the
api-review
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin 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
WalkthroughDocumentation and schema description updates across Go source files and CRD manifests clarify field semantics, expand definitions for Agent, InfraEnv, and AgentClusterInstall types, and mark VIP fields as deprecated in favor of plural variants for dual-stack configurations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/crd/bases/extensions.hive.openshift.io_agentclusterinstalls.yaml`:
- Around line 58-62: The description for APIVIP and analogous singular VIP
fields is misleading: update the YAML description text so it documents that when
both singular (APIVIP/IngressVIP) and plural (apiVIPs/ingressVIPs) fields are
provided they must match (backwards-compatibility validation will reject
mismatches) rather than being silently ignored; reference the backend validation
behavior implemented in HandleApiVipBackwardsCompatibility and
HandleIngressVipBackwardsCompatibility and state that the singular fields are
deprecated but still validated for equality with the corresponding entry in the
plural lists when both are set.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f450df5-f227-4a4a-baa5-9123901c39b5
⛔ Files ignored due to path filters (3)
vendor/github.com/openshift/assisted-service/api/hiveextension/v1beta1/agentclusterinstall_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/agent_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/infraenv_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (12)
api/hiveextension/v1beta1/agentclusterinstall_types.goapi/v1beta1/agent_types.goapi/v1beta1/infraenv_types.goconfig/crd/bases/agent-install.openshift.io_agents.yamlconfig/crd/bases/agent-install.openshift.io_infraenvs.yamlconfig/crd/bases/extensions.hive.openshift.io_agentclusterinstalls.yamlconfig/crd/resources.yamlconfig/manifests/bases/assisted-service-operator.clusterserviceversion.yamldeploy/olm-catalog/manifests/agent-install.openshift.io_agents.yamldeploy/olm-catalog/manifests/agent-install.openshift.io_infraenvs.yamldeploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yamldeploy/olm-catalog/manifests/extensions.hive.openshift.io_agentclusterinstalls.yaml
| description: |- | ||
| APIVIP is the virtual IP address used to reach the OpenShift cluster's API server. | ||
| Deprecated: use apiVIPs instead to support dual-stack configurations. This field is ignored | ||
| when apiVIPs is set. For single-stack clusters, either field can be used; for dual-stack | ||
| clusters, use apiVIPs. |
There was a problem hiding this comment.
Clarify VIP backward-compatibility behavior (current text is misleading)
The docs currently say singular VIP fields are “ignored” when plural fields are set. In practice, the backend validates both and rejects mismatches (internal/cluster/validations/validations.go, HandleApiVipBackwardsCompatibility and HandleIngressVipBackwardsCompatibility). Please document the required-match behavior when both are provided.
Proposed wording update
- Deprecated: use apiVIPs instead to support dual-stack configurations. This field is ignored
- when apiVIPs is set. For single-stack clusters, either field can be used; for dual-stack
+ Deprecated: use apiVIPs instead to support dual-stack configurations. If both fields are set,
+ apiVIP must match the first element of apiVIPs; otherwise the request is rejected. For single-stack
+ clusters, either field can be used; for dual-stack
clusters, use apiVIPs.- Deprecated: use ingressVIPs instead to support dual-stack configurations. This field is ignored
- when ingressVIPs is set. For single-stack clusters, either field can be used; for dual-stack
+ Deprecated: use ingressVIPs instead to support dual-stack configurations. If both fields are set,
+ ingressVIP must match the first element of ingressVIPs; otherwise the request is rejected. For single-stack
+ clusters, either field can be used; for dual-stack
clusters, use ingressVIPs.As per coding guidelines, Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Also applies to: 346-350, 635-639
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/crd/bases/extensions.hive.openshift.io_agentclusterinstalls.yaml`
around lines 58 - 62, The description for APIVIP and analogous singular VIP
fields is misleading: update the YAML description text so it documents that when
both singular (APIVIP/IngressVIP) and plural (apiVIPs/ingressVIPs) fields are
provided they must match (backwards-compatibility validation will reject
mismatches) rather than being silently ignored; reference the backend validation
behavior implemented in HandleApiVipBackwardsCompatibility and
HandleIngressVipBackwardsCompatibility and state that the singular fields are
deprecated but still validated for equality with the corresponding entry in the
plural lists when both are set.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10124 +/- ##
==========================================
- Coverage 44.28% 44.27% -0.01%
==========================================
Files 415 415
Lines 72709 72709
==========================================
- Hits 32199 32194 -5
- Misses 37599 37602 +3
- Partials 2911 2913 +2 🚀 New features to boost your workflow:
|
|
@carbonin: 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. |
2 participants
Update CRDs with more useful API documentation. This focuses on the fields called out in ACM-32735 specifically.
List all the issues related to this PR
Resolves https://redhat.atlassian.net/browse/ACM-32735
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist