CORS-4417: Add UniverseDomain field to GCPPlatformStatus#2933
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@barbacbd: This pull request references CORS-4417 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 "5.0.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. |
|
Hello @barbacbd! Some important instructions when contributing to openshift/api: |
|
Documented in Enhancement openshift/enhancements#1977 |
openshift-ci
Bot
added
the
size/S
|
[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 |
|
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:
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 (18)
📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughAdded an optional, feature-gated Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✨ 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. 🔧 Checkov (3.3.8)config/v1/tests/infrastructures.config.openshift.io/GCPSovereignCloudInstall.yamlTraceback (most recent call last): payload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlTraceback (most recent call last): payload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlTraceback (most recent call last):
🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
a55f248 to
8d5237b
Compare
|
|
||
| // universeDomain is the GCP universe domain detected from credentials. | ||
| // Populated by the installer for informational/observability purposes. | ||
| // Components should NOT read this field - they should detect universe domain |
There was a problem hiding this comment.
Perhaps we should reword this slightly. The intent is that components with GCP credentials should read universe domain from them—the creds are the source of truth and reading from them is more ergononic.
We provide this field for components that may not have creds (MCO looks like it will need the field) as well as general informational purposes.
|
/test api-review |
|
/test evals-api-review |
![openshift-api-review-bot[bot]](https://avatars.githubusercontent.com/u/84759374?s=60&v=4){kind=small}
There was a problem hiding this comment.
API review complete — no issues found.
The new UniverseDomain field on GCPPlatformStatus is well-documented:
- Field purpose and source are clearly described.
- Optional behavior is documented: "When omitted, standard public GCP (googleapis.com) is assumed."
- Both
MinLength=1andMaxLength=253constraints are documented in the comment ("must be between 1 and 253 characters in length"). The combination of+optional/omitemptywithMinLength=1is correct — the field may be absent, but if set must be non-empty. - No undocumented cross-field relationships or CEL expressions.
zz_generated.swagger_doc_generated.gois properly updated to match the field comment.
API ReviewIssues were found and posted as inline comments on this PR.
|
|
/test verify-hypershift-integration |
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| // +optional | ||
| UniverseDomain string `json:"universeDomain,omitempty"` |
There was a problem hiding this comment.
Could we add any validation here to check that this is a valid domain? Is this just the domain and not a URL?
I wonder if this has to be compliant with the RFC DNS1123 subdomain validation?
openshift-ci
Bot
added
size/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml (1)
2238-2246: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd
universeDomainto this GCP status schema.
spec.infra.status.platformStatus.gcplacks the new field entirely, so this TechPreview ControllerConfig CRD rejects or prunes the status value despite the API contract. Add the same description,maxLength, and DNS-subdomain validation used by the Infrastructure CRDs.🤖 Prompt for 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. In `@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml` around lines 2238 - 2246, Add the universeDomain property to the GCP status schema near projectID in the ControllerConfig CRD, using the same description, maxLength, and DNS-subdomain validation as the Infrastructure CRDs. Ensure it is defined under spec.infra.status.platformStatus.gcp so status values are preserved and accepted.
🤖 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.
Outside diff comments:
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml`:
- Around line 2238-2246: Add the universeDomain property to the GCP status
schema near projectID in the ControllerConfig CRD, using the same description,
maxLength, and DNS-subdomain validation as the Infrastructure CRDs. Ensure it is
defined under spec.infra.status.platformStatus.gcp so status values are
preserved and accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2659d554-bc8e-45ea-99c9-0186f11fa7a3
⛔ Files ignored due to path filters (18)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/GCPSovereignCloudInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/GCPSovereignCloudInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (13)
config/v1/tests/infrastructures.config.openshift.io/GCPSovereignCloudInstall.yamlconfig/v1/types_infrastructure.gopayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- config/v1/types_infrastructure.go
|
@barbacbd: 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. |
6 participants
No description provided.