CORS-4029: Promote AzureClusterHostedDNS to default#2774
CORS-4029: Promote AzureClusterHostedDNS to default#2774sadasu wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @sadasu! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis pull request introduces Azure cluster-hosted DNS configuration support. It adds a new ✨ 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.11.3)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
[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.
🧹 Nitpick comments (1)
config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml (1)
387-388: Add targeted Azure cloudLoadBalancerConfig validation tests.This expected-output update verifies defaulting, but it would be safer to also cover
dnsTypeimmutability andclusterHostedconditional validation in dedicated cases to reduce regression risk.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml` around lines 387 - 388, Add two focused tests around cloudLoadBalancerConfig: one to assert dnsType immutability and another to assert clusterHosted conditional validation. Specifically, in the tests for the infrastructure OpenShift config (the AAA_ungated.yaml test suite) add a case that attempts to change cloudLoadBalancerConfig.dnsType after creation and expects a validation error (verify behavior of the immutability rule for dnsType), and add a case that supplies clusterHosted=true/false combinations with and without the required platform-specific fields to confirm conditional validation for clusterHosted is enforced. Reference the cloudLoadBalancerConfig, dnsType and clusterHosted fields in the new tests so they explicitly exercise those validations and update expected outputs accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml`:
- Around line 387-388: Add two focused tests around cloudLoadBalancerConfig: one
to assert dnsType immutability and another to assert clusterHosted conditional
validation. Specifically, in the tests for the infrastructure OpenShift config
(the AAA_ungated.yaml test suite) add a case that attempts to change
cloudLoadBalancerConfig.dnsType after creation and expects a validation error
(verify behavior of the immutability rule for dnsType), and add a case that
supplies clusterHosted=true/false combinations with and without the required
platform-specific fields to confirm conditional validation for clusterHosted is
enforced. Reference the cloudLoadBalancerConfig, dnsType and clusterHosted
fields in the new tests so they explicitly exercise those validations and update
expected outputs accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ce88a19d-6671-40ae-8679-7ba976a21be4
⛔ Files ignored due to path filters (5)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (11)
config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yamlfeatures.mdfeatures/features.gopayload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
sadasu
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sadasu: This pull request references CORS-4029 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. |
|
/jira refresh |
|
@sadasu: This pull request references CORS-4029 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. |
|
@JoelSpeed, I would like some guidance on the integration test failures https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_api/2774/pull-ci-openshift-api-master-integration/2036210924280352768 There is another featuregate |
This is passing tests locally for me: patrickdillon@99e7d68 crdName: infrastructures.config.openshift.io
featureGates:
- AzureClusterHostedDNSInstall
- -AzureDualStackInstallmeans something like "run this test where |
Wasn't aware of this syntax to selectively disable featuregates. Thanks! |
Also make it part of OKD.
sadasu
force-pushed
the
azureClusterHostedDNS-default
branch
from
f1fa34a to
c67875d
Compare
|
@sadasu: 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. |
3 participants
No description provided.