CNTRLPLANE-3237: Kms plugin health report clean up#2931
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@p0lyn0mial: This pull request references CNTRLPLANE-3237 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 @p0lyn0mial! 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:
📝 WalkthroughWalkthroughThe KMS health report API fields were renamed from Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
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)
operator/v1/types_kmsencryption.go (1)
31-55: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRename the KMS fixture fields to
keyID/remoteKeyID.
The CRDs now require those names, butoperator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yaml,operator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yaml, andoperator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamlstill usekeyId/kekId, so the fixtures no longer match schema validation.🤖 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 `@operator/v1/types_kmsencryption.go` around lines 31 - 55, Update the KMSEncryption fixture manifests in the three specified test resources to use keyID instead of keyId and remoteKeyID instead of kekId. Keep the fixture values unchanged and align them with the KeyID and RemoteKeyID fields defined in the KMSEncryption type.Source: Learnings
🤖 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 `@operator/v1/types_kmsencryption.go`:
- Around line 31-55: Update the KMSEncryption fixture manifests in the three
specified test resources to use keyID instead of keyId and remoteKeyID instead
of kekId. Keep the fixture values unchanged and align them with the KeyID and
RemoteKeyID fields defined in the KMSEncryption type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 79ef15d4-7b29-4ce9-827a-1d097b115747
⛔ Files ignored due to path filters (15)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/authentications.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/kubeapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (10)
operator/v1/types_kmsencryption.gopayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
64431d9 to
68b30b5
Compare
|
verify-crd-schema, verify-crdify failures are expected but not sure about the other. This looks good to me from feature team POV |
|
Scheduling tests matching the |
68b30b5 to
df6021b
Compare
openshift-ci
Bot
added
size/XL
| LastCheckedTime metav1.Time `json:"lastCheckedTime,omitempty"` | ||
|
|
||
| // kekId refers to the remote KEK id from KMS v2 StatusResponse.key_id. | ||
| // remoteKeyID refers to the remote KEK id from KMS v2 StatusResponse.key_id. |
There was a problem hiding this comment.
| // remoteKeyID refers to the remote KEK id from KMS v2 StatusResponse.key_id. | |
| // remoteKeyID refers to the remote key identifier from KMS v2 StatusResponse.key_id. |
There was a problem hiding this comment.
thanks for the suggestion i will wait for a review from @JoelSpeed before pushing the pr again.
df6021b to
856177b
Compare
|
This looks good to me from feature team POV. |
openshift-ci
Bot
added
the
needs-rebase
856177b to
37fc80f
Compare
openshift-ci
Bot
removed
the
needs-rebase
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)
operator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yaml (1)
297-324: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the remaining health report fields to the new schema.
This fixture still uses
keyIdandkekIdin bothupdatedandexpected, while the regenerated CRD requireskeyIDandremoteKeyID. The request will fail validation because the required renamed fields are missing.Proposed fix
- keyId: key-1 + keyID: key-1 ... - kekId: kek-1 + remoteKeyID: kek-1Apply the same rename in both payloads.
Based on learnings, all consumers and fixtures for renamed CRD fields must switch atomically in the same release.
🤖 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 `@operator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yaml` around lines 297 - 324, Update the health report fields in both the updated payload and expected output to use keyID and remoteKeyID instead of keyId and kekId, preserving their existing values and structure.Source: Learnings
🤖 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 `@operator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yaml`:
- Around line 297-324: Update the health report fields in both the updated
payload and expected output to use keyID and remoteKeyID instead of keyId and
kekId, preserving their existing values and structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 100eb104-4bbb-4f81-a100-97a6cd941c6e
⛔ Files ignored due to path filters (15)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/authentications.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/kubeapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (13)
operator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yamloperator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yamloperator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamloperator/v1/types_kmsencryption.gopayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yaml
- operator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yaml
- operator/v1/types_kmsencryption.go
- operator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yaml
|
Integration tests need update but otherwise LGTM |
37fc80f to
f4a969b
Compare
|
/test integration |
|
PR-Agent: could not find a component named |
|
/lgtm Existing fields being tombstoned as part of a rename, this is acceptable for a tech-preview field |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema, ci/prow/verify-crdify, ci/prow/verify-hypershift-integration These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use 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 kubernetes-sigs/prow repository. |
|
@p0lyn0mial: 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. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, JoelSpeed 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
5 participants
Clean up KMSPluginHealthReport API as discussed in #2916