CNTRLPLANE-3361: make transitMount required#2847
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@flavianmissi: This pull request references CNTRLPLANE-3361 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 @flavianmissi! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/L
|
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 (7)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR updates the Vault KMS encryption configuration in OpenShift to enforce 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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 Comment |
![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)
config/v1/types_kmsencryption.go (1)
194-208:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign
transitMountdocs with required semantics.Line 196 still says “when specified”, but Line 207 marks
transitMountas required. Please update the field comment to explicitly state it is required, then regenerate CRDs so descriptions remain consistent.As per coding guidelines, "All kubebuilder validation markers must be documented in the field's comment, including field optionality, length constraints, value constraints, and advanced validation rules."✏️ Proposed fix
// transitMount specifies the mount path of the Vault Transit engine. + // This field is required. // - // The transit mount must be between 1 and 1024 characters when specified, cannot start or + // The transit mount must be between 1 and 1024 characters, cannot start or // end with a forward slash, cannot contain consecutive forward slashes, and must only contain🤖 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 `@config/v1/types_kmsencryption.go` around lines 194 - 208, Update the TransitMount field comment to remove "when specified" and explicitly state that transitMount is required, and document all kubebuilder validation markers (MinLength, MaxLength, XValidation rules and the +required marker) in the comment to reflect length and value constraints and advanced validation; then regenerate the CRDs so the field description in the CRD matches the updated comment. Refer to the TransitMount field and its kubebuilder markers in types_kmsencryption.go when making these changes.
🤖 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 `@config/v1/types_kmsencryption.go`:
- Around line 194-208: Update the TransitMount field comment to remove "when
specified" and explicitly state that transitMount is required, and document all
kubebuilder validation markers (MinLength, MaxLength, XValidation rules and the
+required marker) in the comment to reflect length and value constraints and
advanced validation; then regenerate the CRDs so the field description in the
CRD matches the updated comment. Refer to the TransitMount field and its
kubebuilder markers in types_kmsencryption.go when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8f5d80e7-6ad3-4c1b-a755-461b2d82982a
⛔ Files ignored due to path filters (7)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (4)
config/v1/types_kmsencryption.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
|
verify-crd-schema and verify-crdify tests are breaking because I'm making an optional field required, hopefully we can them. |
flavianmissi
force-pushed
the
transitmount-required
branch
from
9c028d0 to
086efcd
Compare
|
From KMS feature team POV, changes look good to me |
Just to make sure, regardless of what the vault team is doing behind the scenes, we believe that making this field required is the most reasonable for our API? Just because vault is going to make the field required doesn't mean that we have to as well. |
|
Yes, I think this makes sense for us. If What do y'all think? |
|
I don't have a strong opinion, just wanted to make sure we want to do that regardless of what vault is doing. /lgtm /override ci/prow/verify-crd-schema |
|
@everettraven: Overrode contexts on behalf of everettraven: ci/prow/verify-crd-schema, ci/prow/verify-crdify 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. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven 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
|
@flavianmissi: 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. |
4 participants
also update the tests, and rephrase secret and configmap reference godocs.
background: the vault team is going to make this field required in their plugin (while also removing the default).