CNTRLPLANE-3237: Rename KMSConfig Struct to KMSPluginConfig#2833
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@ardaguclu: 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 @ardaguclu! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/S
|
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 (4)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR renames KMS-related Go types and updates a field type. In 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 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.1)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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 3-7: The exported types were renamed (e.g., KMSConfig and
VaultKMSConfig) and that breaks Go API compatibility; restore backward
compatibility by adding deprecated type aliases for the old names pointing to
the new types in the same package (for example: type KMSConfig =
KMSProviderConfig and type VaultKMSConfig = <newVaultType>), mark them with a
deprecation comment, and ensure the aliases are placed alongside the new
definitions (referencing KMSProviderConfig and the new Vault type names) so
existing consumers can continue to compile.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2d600375-5e12-4803-9a68-fa0b57080c5f
⛔ Files ignored due to path filters (3)
config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (2)
config/v1/types_apiserver.goconfig/v1/types_kmsencryption.go
| // KMSProviderConfig defines the configuration for the KMS instance | ||
| // that will be used with KMS encryption | ||
| // +kubebuilder:validation:XValidation:rule="self.type == 'Vault' ? has(self.vault) : !has(self.vault)",message="vault config is required when kms provider type is Vault, and forbidden otherwise" | ||
| // +union | ||
| type KMSConfig struct { | ||
| type KMSProviderConfig struct { |
There was a problem hiding this comment.
Preserve Go API compatibility for renamed exported types.
Even with stable JSON names, renaming exported types in v1 (KMSConfig, VaultKMSConfig) is a source-breaking change for downstream Go consumers. Please add deprecated type aliases to keep existing integrations compiling.
Proposed backward-compatible alias patch
type KMSProviderConfig struct {
...
}
+// Deprecated: use KMSProviderConfig.
+type KMSConfig = KMSProviderConfig
+
...
type VaultKMSProviderConfig struct {
...
}
+
+// Deprecated: use VaultKMSProviderConfig.
+type VaultKMSConfig = VaultKMSProviderConfigAlso applies to: 125-126
🤖 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 3 - 7, The exported types were
renamed (e.g., KMSConfig and VaultKMSConfig) and that breaks Go API
compatibility; restore backward compatibility by adding deprecated type aliases
for the old names pointing to the new types in the same package (for example:
type KMSConfig = KMSProviderConfig and type VaultKMSConfig = <newVaultType>),
mark them with a deprecation comment, and ensure the aliases are placed
alongside the new definitions (referencing KMSProviderConfig and the new Vault
type names) so existing consumers can continue to compile.
ardaguclu
force-pushed
the
rename-kms-config
branch
from
f3db516 to
d89a472
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
config/v1/types_kmsencryption.go (1)
3-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve Go API compatibility for renamed exported
v1typesLine 7 and Line 126 rename exported symbols (
KMSConfig,VaultKMSConfig) without compatibility aliases. This is source-breaking for Go consumers even if JSON keys are unchanged. Please keep deprecated aliases to avoid downstream compile breaks.Suggested backward-compatible patch
type KMSProviderConfig struct { ... } + +// Deprecated: use KMSProviderConfig. +type KMSConfig = KMSProviderConfig ... type VaultKMSProviderConfig struct { ... } + +// Deprecated: use VaultKMSProviderConfig. +type VaultKMSConfig = VaultKMSProviderConfigAlso applies to: 23-23, 125-126
🤖 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 3 - 7, The exported type names were changed (e.g., KMSConfig -> KMSProviderConfig and VaultKMSConfig -> VaultKMSProviderConfig) and you must add deprecated aliases to preserve Go API compatibility: declare type aliases with the old names pointing to the new structs (e.g., type KMSConfig = KMSProviderConfig and type VaultKMSConfig = VaultKMSProviderConfig) and add a deprecation comment atop each alias; place these aliases in the same package alongside the new types so existing consumers still compile.
🤖 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.
Duplicate comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 3-7: The exported type names were changed (e.g., KMSConfig ->
KMSProviderConfig and VaultKMSConfig -> VaultKMSProviderConfig) and you must add
deprecated aliases to preserve Go API compatibility: declare type aliases with
the old names pointing to the new structs (e.g., type KMSConfig =
KMSProviderConfig and type VaultKMSConfig = VaultKMSProviderConfig) and add a
deprecation comment atop each alias; place these aliases in the same package
alongside the new types so existing consumers still compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 49d00e60-007f-4588-ad11-950ee7e2f295
⛔ Files ignored due to path filters (4)
config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*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 (2)
config/v1/types_apiserver.goconfig/v1/types_kmsencryption.go
✅ Files skipped from review due to trivial changes (1)
- config/v1/types_apiserver.go
|
/lgtm |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
openshift-ci
Bot
added
the
approved
|
After this PR, I learned that it is safe to rename the internal structs as long as we don't change the json fields. Thank you for approval. |
openshift-ci-robot
added
the
verified
|
@ardaguclu: This PR has been marked as verified by 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. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@ardaguclu This API is still tech preview right? If so, I think it's ok to fix the lint issue |
Yes, it is in Tech Preview. Our library go changes that rely on emptiness of |
If it is recommended, I can fix it though. cc'ing @flavianmissi, since he will have a followup PR for validations. |
openshift-ci-robot
removed
the
verified
|
@JoelSpeed lint error is fixed. Could you PTAL one more time?. Thank you |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/lgtm |
|
/hold |
openshift-ci
Bot
added
do-not-merge/hold
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
ardaguclu
force-pushed
the
rename-kms-config
branch
from
8decea9 to
8e78002
Compare
|
@ardaguclu: 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. |
|
/retitle CNTRLPLANE-3237: Rename KMSConfig Struct to KMSPluginConfig |
openshift-ci
Bot
changed the title
ardaguclu
force-pushed
the
rename-kms-config
branch
from
8e78002 to
e0eecb4
Compare
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@ardaguclu: This PR has been marked as verified by 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. |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven, 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 |
|
@ardaguclu: 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. |
4 participants
KMSConfig collides with the upstream KMSConfiguration resource and this creates confusions.
This PR renames KMSConfig Go Struct name to KMSPluginConfig as well as VaultConfig to VaultPluginConfig without changing the json field names.. So this should be a safe change.