CNTRLPLANE-2777: refactor(lib/resourcebuilder): generate interface for client instead of their specific types#1344
Conversation
WalkthroughThis change replaces concrete Kubernetes and OpenShift client types with their corresponding interface equivalents across the resource builder infrastructure. The generation script updates type annotations from pointer references to direct type references, and 11 client field types are updated from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/resourcebuilder/resourcebuilder.go (1)
45-45: Unused client fields detected.
apiregistrationClientv1(line 45) andconfigClientv1(line 48) are declared and initialized but never used in theDo()method. This is a pre-existing issue from the generator configuration (noted in the generator review). Since this file is auto-generated, the fix should be applied inhack/generate-lib-resources.pyby removing the corresponding entries from theclientsdictionary.Also applies to: 48-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resourcebuilder/resourcebuilder.go` at line 45, Remove the unused client entries from the generator so they are not emitted: update hack/generate-lib-resources.py to delete the apiregistrationClientv1 and configClientv1 keys from the clients dictionary so the generator stops declaring/initializing the apiregistrationClientv1 and configClientv1 fields in resourcebuilder.go (they are never referenced in the Do() method); regenerate the file afterward to ensure the unused fields are gone.hack/generate-lib-resources.py (1)
294-304: Interface type names are correct.All interface names match the conventions in k8s client-go and OpenShift client-go packages.
Note:
config/v1(line 295) andapiregistration/v1(line 304) are defined inclientsbut have no corresponding entries intypes. This results in unused client fields (configClientv1,apiregistrationClientv1) in the generated builder struct. Consider removing these entries if they are not needed, or adding corresponding type entries if they should be supported.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-lib-resources.py` around lines 294 - 304, The clients map includes entries for 'github.com/openshift/api/config/v1' and 'k8s.io/kube-aggregator/pkg/apis/apiregistration/v1' but there are no matching entries in the types map, resulting in unused generated builder fields configClientv1 and apiregistrationClientv1; either remove those two client entries from the clients mapping or add corresponding type entries in the types mapping so the generator produces matching types (adjust the clients dict entries shown and/or the types dict to keep clients and types in sync and eliminate the unused builder struct fields).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hack/generate-lib-resources.py`:
- Around line 294-304: The clients map includes entries for
'github.com/openshift/api/config/v1' and
'k8s.io/kube-aggregator/pkg/apis/apiregistration/v1' but there are no matching
entries in the types map, resulting in unused generated builder fields
configClientv1 and apiregistrationClientv1; either remove those two client
entries from the clients mapping or add corresponding type entries in the types
mapping so the generator produces matching types (adjust the clients dict
entries shown and/or the types dict to keep clients and types in sync and
eliminate the unused builder struct fields).
In `@lib/resourcebuilder/resourcebuilder.go`:
- Line 45: Remove the unused client entries from the generator so they are not
emitted: update hack/generate-lib-resources.py to delete the
apiregistrationClientv1 and configClientv1 keys from the clients dictionary so
the generator stops declaring/initializing the apiregistrationClientv1 and
configClientv1 fields in resourcebuilder.go (they are never referenced in the
Do() method); regenerate the file afterward to ensure the unused fields are
gone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8712a449-197b-4489-927d-fe88686e7f41
📒 Files selected for processing (3)
hack/generate-lib-resources.pylib/resourcebuilder/resourcebuilder.golib/resourceread/resourceread.go
…of their specific types Required for building unit tests injecting fake clients
b04ea98 to
a8de48a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/generate-lib-resources.py (1)
294-304: Keepclientsaligned withtypesto avoid dead generated members.
github.com/openshift/api/config/v1andk8s.io/kube-aggregator/pkg/apis/apiregistration/v1are still listed here, but they are not present intypes. The generator therefore still emits and constructs unusedconfigClientv1andapiregistrationClientv1fields inlib/resourcebuilder/resourcebuilder.go. Either prune those entries or filterclient_propertiesto packages declared intypes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-lib-resources.py` around lines 294 - 304, The clients mapping in generate-lib-resources.py contains entries for 'github.com/openshift/api/config/v1' and 'k8s.io/kube-aggregator/pkg/apis/apiregistration/v1' that are not present in the types list, causing unused fields (configClientv1, apiregistrationClientv1) to be generated in lib/resourcebuilder/resourcebuilder.go; fix by either removing those two keys from the clients mapping or, better, filter the client_properties generation to only include packages present in the types collection (i.e., check the package key exists in the types dict before emitting client entries) so generated client fields align with types and eliminate dead members.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hack/generate-lib-resources.py`:
- Around line 294-304: The clients mapping in generate-lib-resources.py contains
entries for 'github.com/openshift/api/config/v1' and
'k8s.io/kube-aggregator/pkg/apis/apiregistration/v1' that are not present in the
types list, causing unused fields (configClientv1, apiregistrationClientv1) to
be generated in lib/resourcebuilder/resourcebuilder.go; fix by either removing
those two keys from the clients mapping or, better, filter the client_properties
generation to only include packages present in the types collection (i.e., check
the package key exists in the types dict before emitting client entries) so
generated client fields align with types and eliminate dead members.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f5e4ad65-5f6f-4204-bd6c-f33a829da858
📒 Files selected for processing (2)
hack/generate-lib-resources.pylib/resourcebuilder/resourcebuilder.go
|
@ingvagabund: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DavidHurta, ingvagabund 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 |
|
@ingvagabund: This pull request references CNTRLPLANE-2777 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 epic 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. |
|
/verified by CI |
|
@ingvagabund: 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. |
|
/retest-required |
Required for building unit tests injecting fake clients
Summary by CodeRabbit