CNTRLPLANE-4008: enable hypershiftlinter and fix test naming - #9271
CNTRLPLANE-4008: enable hypershiftlinter and fix test naming#9271bryan-cox wants to merge 6 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request references CNTRLPLANE-4008 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 task 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/api
Promote golang.org/x/tools from indirect to direct in hack/tools and vendor the analysistest, checker, diff, testenv, and txtar packages needed by the hypershiftlinter analyzer test suites. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add the plugin entry point and settings-based analyzer selection for a custom golangci-lint plugin. The plugin builds as a shared object (.so) that golangci-lint loads at runtime. - plugin.go: BuildAnalyzers with optional enable list via settings - cmd/plugin/main.go: golangci-lint plugin entry point - Makefile: hypershiftlinter.so build target and test-linter target Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bryan-cox 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 |
openshift-ci
Bot
added
area/control-plane-pki-operator
Add 7 analyzers that enforce HyperShift coding conventions from TESTING.md and test/e2e/v2/AGENTS.md: - testcasename: test case names must match "When ..., it should ..." - testfuncname: test functions must use TestXxx or TestXxx_Xxx - sippyannotation: Describe blocks need [sig-hypershift][Jira:Hypershift] - guestcluster: forbids "guest cluster" terminology in e2e tests - contextbackground: forbids context.Background() in e2e tests - vacuouspass: detects loops that pass vacuously on empty collections - ipv6url: forbids fmt.Sprintf for host:port (use net.JoinHostPort) Each analyzer includes analysistest-based unit tests with good/bad testdata fixtures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds seven Possibly related PRs
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9271 +/- ##
==========================================
+ Coverage 45.03% 45.09% +0.05%
==========================================
Files 780 780
Lines 97623 97658 +35
==========================================
+ Hits 43969 44043 +74
+ Misses 50626 50571 -55
- Partials 3028 3044 +16 see 4 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Add test-linter.yaml caller and test-linter-reusable.yaml to run analyzer unit tests on PRs that change linter code. Update lint-reusable.yaml cache key to include hypershiftlinter source files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go (1)
1618-1631: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the EndpointSlice port named by these cases.
The renamed cases claim that both
EndpointsandEndpointSliceports are set. The assertions at Lines 1661-1662 still readendpoints.Subsets[0].Ports[0], so they repeat the legacyEndpointscheck. A regression inendpointSlice.Portscan pass. AssertendpointSlice.Ports[0]with length and nil checks.🤖 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 `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go` around lines 1618 - 1631, Update the assertions in the test cases around the endpoint port expectations to validate endpointSlice.Ports[0] rather than repeating endpoints.Subsets[0].Ports[0]. Add length and nil checks before accessing the slice entry, while preserving the existing expectedPort comparisons for both configured and default ports.hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go (3)
1130-1142: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the autoscaling case name with its input.
This case passes
isAutoscalingNeeded: trueand removes an existingDisableClusterAutoscalerAnnotation. The name says that autoscaling is no longer needed. Rename it to describe the needed-autoscaling path.🤖 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go` around lines 1130 - 1142, Rename the test case in the autoscaling scenarios to reflect that isAutoscalingNeeded is true and the DisableClusterAutoscalerAnnotation is removed. Update only the case name to describe the needed-autoscaling path, leaving its inputs and expected annotations unchanged.
2670-2685: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the missing Secret, not a missing reference.
HostedCluster.Spec.PullSecretreferences"pull-secret". The test omits the Secret and expects a not-found error. Rename the case to state that the referenced pull Secret is unavailable.🤖 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go` around lines 2670 - 2685, Rename the test case in the relevant table-driven test to indicate that the referenced pull Secret is unavailable, rather than saying no pull secret was provided. Keep the existing HostedCluster configuration and expected not-found error unchanged.
2935-2966: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the release transition descriptions.
The first case compares
4.12.1with4.15.0, which is a y-stream transition, not a z-stream transition. The second case uses4.15.0for both values, so it performs no y-stream upgrade. Rename both cases to match their fixtures.Also applies to: 2970-3002
🤖 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go` around lines 2935 - 2966, Update the test case names around the z-stream and y-stream upgrade scenarios to accurately describe their fixtures: the first case is a y-stream transition from 4.12.1 to 4.15.0, while the second should be named for a z-stream upgrade only if its configured versions represent that transition. Rename both cases without changing the test behavior or fixtures.kas-bootstrap/kas_boostrap_test.go (1)
278-348: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the retained boundary version.
The case keeps
4.7.0, which is the last completed entry, and drops only older versions. “Versions after the completed entry” implies that4.7.0is removed. Rename the case to state that the last completed version and newer entries are retained.🤖 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 `@kas-bootstrap/kas_boostrap_test.go` around lines 278 - 348, Rename the test case describing completed cluster history so it states that the last completed version, 4.7.0, and newer entries are retained while only older versions are removed. Update the case name near expectedFeatureGates without changing the test data or behavior.control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go (1)
314-325: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the managed-mode assertion match the test name.
The name says that the KMS container has no workload identity environment variables. The test rejects only
AZURE_CLIENT_ID. It does not checkAZURE_TENANT_IDorAZURE_FEDERATED_TOKEN_FILE. Assert all three variables or narrow the name toAZURE_CLIENT_ID.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go` around lines 314 - 325, Update the managed-mode check in the test case’s `check` function to reject all workload identity environment variables: `AZURE_CLIENT_ID`, `AZURE_TENANT_ID`, and `AZURE_FEDERATED_TOKEN_FILE`. Keep the existing active KMS container lookup and failure behavior unchanged.control-plane-operator/controllers/hostedcontrolplane/v2/kas/config_test.go (1)
368-379: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove or restore the duplicate profiling case.
Lines 368-376 and Lines 379-387 use the same name,
DisableProfiling: trueinput, and expected configuration. The second renamed case adds no coverage. Delete it or restore its intended distinct setup and description.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/config_test.go` around lines 368 - 379, Remove the duplicate profiling test case in the test table near the existing “When profiling is disabled” entry, or restore its intended distinct parameters, expected configuration, and description; ensure each case provides unique coverage.
🟡 Minor comments (24)
cmd/util/params_test.go-105-107 (1)
105-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the boolean parser in the boolean error case.
TestStructdeclares the boolean field asparam4, but this case passesparam5:invalid. It tests an undeclared parameter instead of invalid boolean conversion. Useparam4:invalidor rename the case to describe the unknown-parameter behavior.Suggested fixture update
- paramsStr: "param5:invalid", + paramsStr: "param4:invalid",🤖 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 `@cmd/util/params_test.go` around lines 105 - 107, Update the invalid-boolean test case in TestStruct to pass param4:invalid, matching the declared boolean field and exercising boolean conversion failure; keep the case name and expected error focused on invalid boolean input.Makefile-395-398 (1)
395-398: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the API-only skip message accurate.
When a change touches only
api/**/*.go, the new filter removes all paths andCHANGED_DIRSis empty. The command then prints “No Go files changed” even though Go files changed. Use a message such as “No testable Go packages changed” so CI output reflects the new exclusion.Suggested message update
- echo "No Go files changed relative to $(PULL_BASE_SHA), skipping tests."; \ + echo "No testable Go packages changed relative to $(PULL_BASE_SHA), skipping tests."; \🤖 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 `@Makefile` around lines 395 - 398, Update the empty-CHANGED_DIRS message in the Makefile test-selection logic to say that no testable Go packages changed, accurately covering API-only changes excluded by the filters; leave the filtering and test execution behavior unchanged.hypershift-operator/controllers/nodepool/nodepool_controller_test.go-2716-2716 (1)
2716-2716: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the platform names in these test cases.
The descriptions do not match
NodePool.Spec.Platform.Type:
- Line 2716 says AWS, but the case uses
hyperv1.AgentPlatform.- Line 2728 says None, but the case uses
hyperv1.AgentPlatform.- Line 2740 says Agent, but the case uses
hyperv1.NonePlatform.- Line 2752 says Agent, but the case uses
hyperv1.NonePlatform.Rename the cases to describe the configured platform.
Also applies to: 2728-2728, 2740-2740, 2752-2752
🤖 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 `@hypershift-operator/controllers/nodepool/nodepool_controller_test.go` at line 2716, Correct the test case names in the platform validation table so each description matches its configured NodePool.Spec.Platform.Type: describe the AgentPlatform cases as Agent and the NonePlatform cases as None, updating the cases currently labeled AWS or Agent incorrectly while preserving the test logic.hypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.go-645-645 (1)
645-645: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
DataVolumein these test names.The test creates and lists
v1beta1.DataVolumeobjects.PVCis a different Kubernetes resource. Rename the cases at Lines 645 and 652 so the test output describes the resource under test.Also applies to: 652-652
🤖 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 `@hypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.go` at line 645, Rename the test cases at the entries currently describing PVCs to refer to DataVolume instead, including the cases around “When no existing PVC exists” and its counterpart. Keep the test behavior unchanged and ensure the names accurately match the v1beta1.DataVolume resources being created and listed.support/netutil/iputil_test.go-17-25 (1)
17-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the first usable IP.
The expected values at Line 19 and Line 25 are
192.168.1.1and2000::1. The first addresses in those CIDRs are192.168.1.0and2000::. Rename both cases to say “first usable IP” so the descriptions matchFirstUsableIPand the assertions.🤖 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 `@support/netutil/iputil_test.go` around lines 17 - 25, Rename the two test case descriptions in the FirstUsableIP tests to say “first usable IP” instead of “first ip of the network range,” while preserving the existing CIDRs and expected values.support/azureutil/azureutil_test.go-794-794 (1)
794-794: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert which Secret is created for the disabled capability.
The case at Line 794 expects the disabled
ingresscredential to be skipped. It only checks that one Secret was created. The validator does not fail wheningress-credsis created because it validates fields only fordisk-csi. Assert thatdisk-credsexists andingress-credsdoes not exist.🤖 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 `@support/azureutil/azureutil_test.go` at line 794, Strengthen the test case “When capability is disabled it should skip creating secrets” by asserting that the created Secret is disk-creds and explicitly asserting that ingress-creds does not exist. Update the test’s Secret validation so it checks both the expected disk-csi credential and absence of the disabled ingress credential.support/events/message_test.go-57-63 (1)
57-63: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename “info events” to “normal events” in the test name.
The fixture uses
corev1.EventTypeNormal.🤖 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 `@support/events/message_test.go` around lines 57 - 63, Rename the test case name in the events table from “info events” to “normal events” to match the corev1.EventTypeNormal fixture, without changing its setup or expected result.support/etcd/shards_test.go-20-23 (1)
20-23: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign shard assertions with their descriptions.
The nil cases only check length, so they do not distinguish a nil slice from an empty slice.
TestUnmanagedEffectiveShardsalso does not check default or configured shard identities. Add these assertions, or weaken the test names to describe only counts.🤖 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 `@support/etcd/shards_test.go` around lines 20 - 23, Update support/etcd/shards_test.go at lines 20-23, 92-95, and 97-105: strengthen the nil-case assertions to distinguish a nil slice from an empty slice, and extend TestUnmanagedEffectiveShards assertions to verify the expected default and configured shard identities; alternatively, weaken the affected test names to describe count-only validation.Source: Coding guidelines
control-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert_test.go-938-938 (1)
938-938: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the proxy behavior, not the whole transport.
The test still loads
fakeCertCADataForConfigMapintotr.TLSClientConfig.RootCAswhen proxy configuration is absent. Therefore, the transport is modified. Rename this case to state that it does not configure a proxy.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert_test.go` at line 938, Rename the test case identified by “When no proxy configuration is provided” to describe only the absence of proxy configuration, such as asserting that no proxy is configured. Do not claim the transport remains unmodified, since fakeCertCADataForConfigMap still changes tr.TLSClientConfig.RootCAs.cmd/cluster/kubevirt/create_test.go-137-145 (1)
137-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRename the KubeVirt fixtures to match
CompareWithFixturepaths.sanitizeFilename(t.Name())produces..._provided_it..., but both fixtures use..._provided__it...; remove the extra underscore beforeit.🤖 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 `@cmd/cluster/kubevirt/create_test.go` around lines 137 - 145, Rename the KubeVirt fixture files for the tests around the minimal-flags and complex-configuration cases so their names match the paths generated by CompareWithFixture and sanitizeFilename(t.Name()). Remove the extra underscore before “it” in both fixture filenames, without changing the test logic.hypershift-operator/controllers/hostedclustersizing/hostedclustersizing_controller_test.go-398-398 (1)
398-398: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not describe status conditions as tags.
Line 398 and Line 460 say the cluster is “tagged with” HCCO reporting or KAS unavailability. The fixtures use a HostedCluster size label plus HCCO and KAS state from status or injected functions. Rename both cases to describe those conditions directly.
As per coding guidelines, test descriptions must use the
When ... it should ...format.Proposed test names
- name: "When previously computed and tagged with HCCO reporting node count it should transition", + name: "When previously computed and a size label is present while HCCO reports node count it should transition", ... - name: "When previously computed and tagged with KAS unavailable it should transition", + name: "When previously computed and KAS is unavailable it should transition",Also applies to: 460-460
🤖 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 `@hypershift-operator/controllers/hostedclustersizing/hostedclustersizing_controller_test.go` at line 398, Rename the test cases at the “When previously computed...” and corresponding KAS-unavailability fixture to describe HCCO reporting node count and KAS unavailability as status or injected state, not tags. Preserve the `When ... it should ...` naming format and leave the test behavior unchanged.Source: Coding guidelines
hypershift-operator/controllers/hostedcluster/validations/ocpapiserver_test.go-75-75 (1)
75-75: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winName the missing KAS state, not a missing custom certificate.
Line 75 configures and adds
customCertSecretto the fake client. The omitted objects are the KAS secrets intt.secrets. Rename the case to state that KAS secrets are absent while the custom certificate configuration is valid.As per coding guidelines, test descriptions must use the
When ... it should ...format.Proposed test name
- name: "When custom serving cert is not deployed with valid configuration it should not return errors", + name: "When KAS secrets are absent and custom serving cert configuration is valid it should not return an error",🤖 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 `@hypershift-operator/controllers/hostedcluster/validations/ocpapiserver_test.go` at line 75, Rename the test case in the relevant test table to describe that KAS secrets are absent while the custom certificate configuration is valid, using the required “When ... it should ...” format; leave the test setup and behavior unchanged.Source: Coding guidelines
hypershift-operator/controllers/hostedcluster/validations/ocpapiserver_test.go-365-368 (1)
365-368: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the wildcard's one-label match.
Line 365 uses
dnsName: "baz.foo.bar.com"andpattern: "*.foo.bar.com". The wildcard matches the singlebazlabel. It does not match multiple levels. Update the name to avoid documenting incorrect wildcard behavior.As per coding guidelines, test descriptions must use the
When ... it should ...format.Proposed test name
- name: "When wildcard pattern matches multiple levels it should return true", + name: "When wildcard pattern matches one level of a multi-level domain it should return true",🤖 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 `@hypershift-operator/controllers/hostedcluster/validations/ocpapiserver_test.go` around lines 365 - 368, Rename the test case identified by the “When wildcard pattern matches multiple levels it should return true” name to describe that the wildcard matches exactly one label, while preserving the existing dnsName, pattern, and expected values. Keep the description in the required “When ... it should ...” format.Source: Coding guidelines
hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go-262-262 (1)
262-262: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLimit the test name to this reconciler's credential secrets.
Line 262 says the test creates “all credential secrets.” The fixture expects three secrets and explicitly excludes disk and file secrets because another component manages them. Rename the case to describe the secrets managed by this reconciler.
As per coding guidelines, test descriptions must use the
When ... it should ...format.Proposed test name
- name: "When self-managed Azure has workload identities it should create all credential secrets", + name: "When self-managed Azure has workload identities it should create the credential secrets managed by this reconciler",🤖 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 `@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go` at line 262, Rename the test case near the self-managed Azure workload identities fixture to use the “When ... it should ...” format and describe only the credential secrets managed by this reconciler, avoiding wording that implies disk and file secrets are included.Source: Coding guidelines
hypershift-operator/controllers/hostedcluster/metrics/metrics_test.go-1029-1042 (1)
1029-1042: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the expiry timestamp metric, not certificate validity.
Line 1029, Line 1035, and Line 1042 are in
TestProxyCAExpiry. The assertions useProxyCAExpiryTimestampNameand compare certificateNotAftertimestamps. Rename these cases to describe the expiry timestamp.As per coding guidelines, test descriptions must use the
When ... it should ...format.Proposed test names
- name: "When cluster is not setting a CA bundle it should not report validity", + name: "When cluster is not setting a CA bundle it should not report an expiry timestamp", ... - name: "When the configured certificates are expired it should report the CA as invalid", + name: "When the configured certificate is expired it should report its expiry timestamp", ... - name: "When the configured certificates are valid it should report the CA as valid", + name: "When the configured certificate is valid it should report its expiry timestamp",🤖 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 `@hypershift-operator/controllers/hostedcluster/metrics/metrics_test.go` around lines 1029 - 1042, The test case names in TestProxyCAExpiry should describe reporting the proxy CA expiry timestamp rather than certificate validity. Rename the affected cases using the “When ... it should ...” format, covering no configured CA bundle, expired certificates, and valid certificates, while leaving the assertions unchanged.Source: Coding guidelines
cmd/nodepool/openstack/create_test.go-97-104 (1)
97-104: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun the renamed validation cases as subtests.
test.nameis populated in the changed cases, but the loop at Line 114 validates each case directly. It never callst.Run(test.name, ...). The descriptions therefore do not appear in test output, and failures do not identify the failing scenario.Wrap the existing validation body in
t.Run(test.name, func(subtest *testing.T) { ... }). Usesubtestto avoid shadowing the outert.The PR objective is to make test names useful and consistent.
As per coding guidelines:
**/*.gocode should avoid variable shadowing.🤖 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 `@cmd/nodepool/openstack/create_test.go` around lines 97 - 104, Wrap each validation case in the test loop with t.Run(test.name, func(subtest *testing.T) { ... }), and use subtest for the existing validation and assertion calls. Preserve the current test logic while avoiding shadowing the outer t variable so the renamed scenarios appear in test output.Source: Coding guidelines
cmd/install/install_test.go-650-662 (1)
650-662: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the platform-selection test descriptions.
The table sets
Options.PlatformsToInstallat Lines 651-653 and 657-659, but the changed names call itPlatformOptions. The combined AWS/Azure name at Line 662 repeats text and does not state one clear expected result. Use the actual field name and describe the selected platforms.Suggested test-name fix
- name: "When PlatformOptions is set to Azure, it should include only Azure CAPI CRDs", + name: "When PlatformsToInstall contains only azure, it should include only Azure CAPI CRDs", ... - name: "When PlatformOptions is set to AWS, it should include only AWS CAPI CRDs", + name: "When PlatformsToInstall contains only aws, it should include only AWS CAPI CRDs", ... - name: "When PlatformOptions is set to AWS,Azure, it should include only AWS When PlatformOptions is set to AWS,Azure, only AWS & Azure CAPI CRDs it should be present Azure CAPI CRDs", + name: "When PlatformsToInstall contains aws and azure, it should include only AWS and Azure CAPI CRDs",The PR objective is to standardize test names and descriptions. Keep each description aligned with its table input.
As per coding guidelines:
**/*_test.gotest cases must use theWhen ... it should ...format.🤖 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 `@cmd/install/install_test.go` around lines 650 - 662, Update the affected table-test names to use the actual Options.PlatformsToInstall field and the required “When ... it should ...” format. Rewrite the combined AWS/Azure description so it clearly states that both AWS and Azure CAPI CRDs are included, while keeping the Azure-only and AWS-only descriptions aligned with their respective inputs.Source: Coding guidelines
hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go-6017-6021 (1)
6017-6021: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the large-port case name match the assertion.
expectErrorisfalse, and the comment states thatparseNodePortRangedoes not validate port limits. The name currently says that the case should return an error. Rename it to state that the range parses without port-limit 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go` around lines 6017 - 6021, Rename the test case in the parseNodePortRange table from implying an error for an oversized port range to stating that the range parses without port-limit validation, matching expectError: false and its existing comment.hypershift-operator/controllers/sharedingress/router_test.go-31-31 (1)
31-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not claim a ConfigMap fixture that this test does not provide.
The test constructs only
args.deployment. It does not create or pass a ConfigMap. Remove “config map” from the name, or add the ConfigMap fixture ifReconcileRouterDeploymentmust validate one.🤖 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 `@hypershift-operator/controllers/sharedingress/router_test.go` at line 31, Update the test case name around ReconcileRouterDeployment to remove the claim that a valid ConfigMap is provided, since the fixture only supplies args.deployment; alternatively, add and pass the required ConfigMap fixture if validation is part of the intended test scenario.ignition-server/controllers/tokensecret_controller_test.go-385-397 (1)
385-397: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign rotation names with the half-TTL fixtures.
The first case uses
timeLivedHalfTTL, which isttl / 2, not a value greater than or equal tottl. The third case usesttl / 2 - 1. Rename the cases to describe the>= half TTLand< half TTLboundary.🤖 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 `@ignition-server/controllers/tokensecret_controller_test.go` around lines 385 - 397, Rename the affected test case descriptions in the rotation test table to match the fixtures: describe timeLivedHalfTTL as “>= half TTL” and timeLivedLessThanTTL as “< half TTL,” while leaving the test values and expected needRotation results unchanged.hypershift-operator/controllers/uwmtelemetry/uwm_telemetry_test.go-372-373 (1)
372-373: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winVerify the “without changes” outcome.
The
validatecallback is empty for this case. The test cannot detect changes to the fake client, although the name promises that reconciliation makes no changes. Add an assertion for the unchanged state, or narrow the name to claim only successful reconciliation.🤖 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 `@hypershift-operator/controllers/uwmtelemetry/uwm_telemetry_test.go` around lines 372 - 373, Update the test case named “When there is no monitoring namespace it should succeed without changes” so its validate callback asserts the fake client remains unchanged after reconciliation, or rename the case to describe success without claiming no changes. Ensure the test name and validation behavior consistently express the intended outcome.control-plane-operator/controllers/hostedcontrolplane/v2/kas/params_test.go-32-45 (1)
32-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the generated advertise address.
The renamed cases claim default and configured advertise-address behavior. Lines 72-76 never compare
p.AdvertiseAddress. They compare the test input withexpectedAddress, and the default case skips the check.Proposed assertion fix
- if len(test.advertiseAddress) > 0 { - g.Expect(test.advertiseAddress).To(Equal(test.expectedAddress)) - } + g.Expect(p.AdvertiseAddress).To(Equal(test.expectedAddress))Also applies to: 72-76
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/params_test.go` around lines 32 - 45, Update the test assertions around the generated parameters to compare p.AdvertiseAddress with expectedAddress in every advertise-address case, including the default case; do not compare the input advertiseAddress to the expected value or skip the default assertion.control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert_test.go-938-938 (1)
938-938: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the no-proxy case to match the assertion.
The test supplies CA data and later checks
tr.TLSClientConfig.RootCAs. Therefore, it does modify the transport. Only proxy selection remains unset.Proposed name update
- name: "When no proxy configuration is provided, it should not modify the transport", + name: "When no proxy configuration is provided, it should not configure a proxy",🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert_test.go` at line 938, Rename the test case currently named “When no proxy configuration is provided, it should not modify the transport” to describe that proxy selection remains unset while CA data modifies the transport; update only the case name and keep its existing assertions unchanged.control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go-616-616 (1)
616-616: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the misleading comment. The
expectedStatusand test name are correct: shared ingress handles public access and Swift networking handles private access, so neither router is needed.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go` at line 616, Update the test description for “When ARO cluster uses Swift with public and private topology, it should use shared ingress” to accurately state that neither router is needed, while preserving the existing expectedStatus.
🧹 Nitpick comments (3)
control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller_test.go (1)
687-697: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign these descriptions with the assertions.
The nameserver-format cases claim that the formats are accepted, but the test only checks that each string is non-empty. The error cases claim that reconciliation continues, but the test does not call reconciliation and
assert.Contains(..., "")succeeds for any non-nil error. Exercise the production paths and assert the outcomes, or narrow the names to the checks that the tests perform.Also applies to: 728-761
🤖 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 `@control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller_test.go` around lines 687 - 697, The nameserver-format and error-case test descriptions in the relevant table-driven tests do not match their weak assertions. Update these cases to exercise the production validation/reconciliation paths and assert the expected acceptance or continuation behavior, ensuring error assertions verify meaningful content rather than allowing any non-nil error; alternatively, rename the cases to accurately describe only the checks currently performed.hypershift-operator/controllers/nodepool/config_test.go (1)
694-694: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not couple test execution to display names. These cases use table names as executable switches. A description-only rename can disable or alter assertions.
hypershift-operator/controllers/nodepool/config_test.go#L694-L694: replace thetc.namecomparison with an explicitisBaseCasefield.hypershift-operator/controllers/nodepool/nodepool_controller_test.go#L3681-L3681: replace the condition-ordertt.namecomparison with an explicit table flag.hypershift-operator/controllers/nodepool/nodepool_controller_test.go#L3722-L3722: replace the early-exittt.namecomparison with an explicit table flag.🤖 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 `@hypershift-operator/controllers/nodepool/config_test.go` at line 694, Decouple table-test control flow from display names by adding an explicit base-case flag to the relevant test-case structs and using it instead of name comparisons: update config_test.go:694-694 for the base-case hash assertion, nodepool_controller_test.go:3681-3681 for condition ordering, and nodepool_controller_test.go:3722-3722 for the early exit; set the flag only on the intended base-case entries while preserving all existing assertions.pkg/featuregates/featuregates_test.go (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMatch the documented test-name convention.
These names omit the comma in
When ..., it should ...and usefeaturesetas one word. Use consistent wording.Proposed name updates
- name: "When a feature gate has no featureset it should never be enabled", + name: "When a feature gate has no feature set, it should never be enabled", - name: "When featuregates have specific featureset enablement it should only enable in explicitly enabled featuresets", + name: "When feature gates have specific feature-set enablement, it should only enable in explicitly enabled feature sets", - t.Run("When an unknown featureset is configured it should return an error", func(t *testing.T) { + t.Run("When an unknown feature set is configured, it should return an error", func(t *testing.T) {As per coding guidelines, unit-test descriptions must use the
When ... it should ...form; the PR objective further specifies the comma-separatedWhen ..., it should ...convention.Also applies to: 42-42, 96-100
🤖 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 `@pkg/featuregates/featuregates_test.go` at line 25, Update the affected test names in the feature-gate tests to follow the documented “When ..., it should ...” convention, including the comma after the condition and spelling “feature set” as two words. Apply this consistently to the cases around the no-feature-set test and the additional referenced test names.Source: Coding guidelines
🤖 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 @.golangci.yml:
- Around line 16-21: Update the CI setup in lint-reusable.yaml to provision
hypershiftlinter.so at hack/tools/bin/hypershiftlinter.so, alongside the
existing golangci-lint and kube-api-linter.so copies from /opt/lint-tools, so
the custom hypershiftlinter configured in .golangci.yml is available before make
lint runs.
---
Outside diff comments:
In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/config_test.go`:
- Around line 368-379: Remove the duplicate profiling test case in the test
table near the existing “When profiling is disabled” entry, or restore its
intended distinct parameters, expected configuration, and description; ensure
each case provides unique coverage.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go`:
- Around line 314-325: Update the managed-mode check in the test case’s `check`
function to reject all workload identity environment variables:
`AZURE_CLIENT_ID`, `AZURE_TENANT_ID`, and `AZURE_FEDERATED_TOKEN_FILE`. Keep the
existing active KMS container lookup and failure behavior unchanged.
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go`:
- Around line 1618-1631: Update the assertions in the test cases around the
endpoint port expectations to validate endpointSlice.Ports[0] rather than
repeating endpoints.Subsets[0].Ports[0]. Add length and nil checks before
accessing the slice entry, while preserving the existing expectedPort
comparisons for both configured and default ports.
In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 1130-1142: Rename the test case in the autoscaling scenarios to
reflect that isAutoscalingNeeded is true and the
DisableClusterAutoscalerAnnotation is removed. Update only the case name to
describe the needed-autoscaling path, leaving its inputs and expected
annotations unchanged.
- Around line 2670-2685: Rename the test case in the relevant table-driven test
to indicate that the referenced pull Secret is unavailable, rather than saying
no pull secret was provided. Keep the existing HostedCluster configuration and
expected not-found error unchanged.
- Around line 2935-2966: Update the test case names around the z-stream and
y-stream upgrade scenarios to accurately describe their fixtures: the first case
is a y-stream transition from 4.12.1 to 4.15.0, while the second should be named
for a z-stream upgrade only if its configured versions represent that
transition. Rename both cases without changing the test behavior or fixtures.
In `@kas-bootstrap/kas_boostrap_test.go`:
- Around line 278-348: Rename the test case describing completed cluster history
so it states that the last completed version, 4.7.0, and newer entries are
retained while only older versions are removed. Update the case name near
expectedFeatureGates without changing the test data or behavior.
---
Minor comments:
In `@cmd/cluster/kubevirt/create_test.go`:
- Around line 137-145: Rename the KubeVirt fixture files for the tests around
the minimal-flags and complex-configuration cases so their names match the paths
generated by CompareWithFixture and sanitizeFilename(t.Name()). Remove the extra
underscore before “it” in both fixture filenames, without changing the test
logic.
In `@cmd/install/install_test.go`:
- Around line 650-662: Update the affected table-test names to use the actual
Options.PlatformsToInstall field and the required “When ... it should ...”
format. Rewrite the combined AWS/Azure description so it clearly states that
both AWS and Azure CAPI CRDs are included, while keeping the Azure-only and
AWS-only descriptions aligned with their respective inputs.
In `@cmd/nodepool/openstack/create_test.go`:
- Around line 97-104: Wrap each validation case in the test loop with
t.Run(test.name, func(subtest *testing.T) { ... }), and use subtest for the
existing validation and assertion calls. Preserve the current test logic while
avoiding shadowing the outer t variable so the renamed scenarios appear in test
output.
In `@cmd/util/params_test.go`:
- Around line 105-107: Update the invalid-boolean test case in TestStruct to
pass param4:invalid, matching the declared boolean field and exercising boolean
conversion failure; keep the case name and expected error focused on invalid
boolean input.
In `@control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go`:
- Line 616: Update the test description for “When ARO cluster uses Swift with
public and private topology, it should use shared ingress” to accurately state
that neither router is needed, while preserving the existing expectedStatus.
In
`@control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert_test.go`:
- Line 938: Rename the test case currently named “When no proxy configuration is
provided, it should not modify the transport” to describe that proxy selection
remains unset while CA data modifies the transport; update only the case name
and keep its existing assertions unchanged.
In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/params_test.go`:
- Around line 32-45: Update the test assertions around the generated parameters
to compare p.AdvertiseAddress with expectedAddress in every advertise-address
case, including the default case; do not compare the input advertiseAddress to
the expected value or skip the default assertion.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert_test.go`:
- Line 938: Rename the test case identified by “When no proxy configuration is
provided” to describe only the absence of proxy configuration, such as asserting
that no proxy is configured. Do not claim the transport remains unmodified,
since fakeCertCADataForConfigMap still changes tr.TLSClientConfig.RootCAs.
In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 6017-6021: Rename the test case in the parseNodePortRange table
from implying an error for an oversized port range to stating that the range
parses without port-limit validation, matching expectError: false and its
existing comment.
In
`@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go`:
- Line 262: Rename the test case near the self-managed Azure workload identities
fixture to use the “When ... it should ...” format and describe only the
credential secrets managed by this reconciler, avoiding wording that implies
disk and file secrets are included.
In `@hypershift-operator/controllers/hostedcluster/metrics/metrics_test.go`:
- Around line 1029-1042: The test case names in TestProxyCAExpiry should
describe reporting the proxy CA expiry timestamp rather than certificate
validity. Rename the affected cases using the “When ... it should ...” format,
covering no configured CA bundle, expired certificates, and valid certificates,
while leaving the assertions unchanged.
In
`@hypershift-operator/controllers/hostedcluster/validations/ocpapiserver_test.go`:
- Line 75: Rename the test case in the relevant test table to describe that KAS
secrets are absent while the custom certificate configuration is valid, using
the required “When ... it should ...” format; leave the test setup and behavior
unchanged.
- Around line 365-368: Rename the test case identified by the “When wildcard
pattern matches multiple levels it should return true” name to describe that the
wildcard matches exactly one label, while preserving the existing dnsName,
pattern, and expected values. Keep the description in the required “When ... it
should ...” format.
In
`@hypershift-operator/controllers/hostedclustersizing/hostedclustersizing_controller_test.go`:
- Line 398: Rename the test cases at the “When previously computed...” and
corresponding KAS-unavailability fixture to describe HCCO reporting node count
and KAS unavailability as status or injected state, not tags. Preserve the `When
... it should ...` naming format and leave the test behavior unchanged.
In `@hypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.go`:
- Line 645: Rename the test cases at the entries currently describing PVCs to
refer to DataVolume instead, including the cases around “When no existing PVC
exists” and its counterpart. Keep the test behavior unchanged and ensure the
names accurately match the v1beta1.DataVolume resources being created and
listed.
In `@hypershift-operator/controllers/nodepool/nodepool_controller_test.go`:
- Line 2716: Correct the test case names in the platform validation table so
each description matches its configured NodePool.Spec.Platform.Type: describe
the AgentPlatform cases as Agent and the NonePlatform cases as None, updating
the cases currently labeled AWS or Agent incorrectly while preserving the test
logic.
In `@hypershift-operator/controllers/sharedingress/router_test.go`:
- Line 31: Update the test case name around ReconcileRouterDeployment to remove
the claim that a valid ConfigMap is provided, since the fixture only supplies
args.deployment; alternatively, add and pass the required ConfigMap fixture if
validation is part of the intended test scenario.
In `@hypershift-operator/controllers/uwmtelemetry/uwm_telemetry_test.go`:
- Around line 372-373: Update the test case named “When there is no monitoring
namespace it should succeed without changes” so its validate callback asserts
the fake client remains unchanged after reconciliation, or rename the case to
describe success without claiming no changes. Ensure the test name and
validation behavior consistently express the intended outcome.
In `@ignition-server/controllers/tokensecret_controller_test.go`:
- Around line 385-397: Rename the affected test case descriptions in the
rotation test table to match the fixtures: describe timeLivedHalfTTL as “>= half
TTL” and timeLivedLessThanTTL as “< half TTL,” while leaving the test values and
expected needRotation results unchanged.
In `@Makefile`:
- Around line 395-398: Update the empty-CHANGED_DIRS message in the Makefile
test-selection logic to say that no testable Go packages changed, accurately
covering API-only changes excluded by the filters; leave the filtering and test
execution behavior unchanged.
In `@support/azureutil/azureutil_test.go`:
- Line 794: Strengthen the test case “When capability is disabled it should skip
creating secrets” by asserting that the created Secret is disk-creds and
explicitly asserting that ingress-creds does not exist. Update the test’s Secret
validation so it checks both the expected disk-csi credential and absence of the
disabled ingress credential.
In `@support/etcd/shards_test.go`:
- Around line 20-23: Update support/etcd/shards_test.go at lines 20-23, 92-95,
and 97-105: strengthen the nil-case assertions to distinguish a nil slice from
an empty slice, and extend TestUnmanagedEffectiveShards assertions to verify the
expected default and configured shard identities; alternatively, weaken the
affected test names to describe count-only validation.
In `@support/events/message_test.go`:
- Around line 57-63: Rename the test case name in the events table from “info
events” to “normal events” to match the corev1.EventTypeNormal fixture, without
changing its setup or expected result.
In `@support/netutil/iputil_test.go`:
- Around line 17-25: Rename the two test case descriptions in the FirstUsableIP
tests to say “first usable IP” instead of “first ip of the network range,” while
preserving the existing CIDRs and expected values.
---
Nitpick comments:
In
`@control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller_test.go`:
- Around line 687-697: The nameserver-format and error-case test descriptions in
the relevant table-driven tests do not match their weak assertions. Update these
cases to exercise the production validation/reconciliation paths and assert the
expected acceptance or continuation behavior, ensuring error assertions verify
meaningful content rather than allowing any non-nil error; alternatively, rename
the cases to accurately describe only the checks currently performed.
In `@hypershift-operator/controllers/nodepool/config_test.go`:
- Line 694: Decouple table-test control flow from display names by adding an
explicit base-case flag to the relevant test-case structs and using it instead
of name comparisons: update config_test.go:694-694 for the base-case hash
assertion, nodepool_controller_test.go:3681-3681 for condition ordering, and
nodepool_controller_test.go:3722-3722 for the early exit; set the flag only on
the intended base-case entries while preserving all existing assertions.
In `@pkg/featuregates/featuregates_test.go`:
- Line 25: Update the affected test names in the feature-gate tests to follow
the documented “When ..., it should ...” convention, including the comma after
the condition and spelling “feature set” as two words. Apply this consistently
to the cases around the no-feature-set test and the additional referenced test
names.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Enable the hypershiftlinter custom golangci-lint plugin that enforces HyperShift test conventions from TESTING.md and test/e2e/v2/AGENTS.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f6c826b to
414d8e5
Compare
openshift-ci
Bot
added
the
area/ci-tooling
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
hack/tools/hypershiftlinter/plugin.go (1)
47-54: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPreallocate
filteredfor the configured analyzer count.
len(s.Analyzers.Enable)is known before the append loop. Initializefilteredwith that capacity.As per coding guidelines, “Preallocate slice capacity when size is known ahead of time.”
🤖 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 `@hack/tools/hypershiftlinter/plugin.go` around lines 47 - 54, Preallocate the filtered analyzer slice in the analyzer-selection loop by creating filtered with capacity len(s.Analyzers.Enable), while retaining its nil/empty length and existing append behavior.Source: Coding guidelines
hack/tools/hypershiftlinter/analyzers/vacuouspass/vacuouspass.go (2)
68-71: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCopy the inherited slice instead of appending to it.
Line 70 appends to
beforeEachAssertions, which the caller owns. If that slice has spare capacity,appendwrites into the caller's backing array and sibling recursion branches share those elements. The current read pattern hides the effect, because no caller reads past its own length. The behavior breaks as soon as anyone reads the parent slice beyond its original length. Build an independent slice.♻️ Proposed fix
func walkGinkgoBlock(pass *analysis.Pass, body *ast.BlockStmt, beforeEachAssertions []string) { localAssertions := collectBeforeEachAssertions(body) - merged := append(beforeEachAssertions, localAssertions...) + merged := slices.Concat(beforeEachAssertions, localAssertions)
slicesis already imported at line 5.As per coding guidelines: "Always check the capacity when copying or appending slices."
🤖 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 `@hack/tools/hypershiftlinter/analyzers/vacuouspass/vacuouspass.go` around lines 68 - 71, Update walkGinkgoBlock so merged is built in an independent slice rather than appending directly to beforeEachAssertions; use the already imported slices helper to copy the inherited assertions before adding localAssertions, preserving the existing ordering and avoiding shared backing-array mutations.Source: Coding guidelines
94-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the two vacuous-pass checks.
checkBlockForVacuousPassandcheckStmtForVacuousPassrepeat the same four conditions and the same diagnostic message text. An edit to the message in one function silently diverges from the other. Extract one function that takes the range statement and the preceding statements.♻️ Proposed structure
const vacuousPassMessage = "range over .Items without preceding non-empty assertion — add Expect(x.Items).NotTo(BeEmpty()) before the loop" // checkRangeStmt reports a vacuous pass for stmt if it ranges over a .Items // selector, asserts inside the loop, and has no preceding non-empty assertion. func checkRangeStmt(pass *analysis.Pass, stmt ast.Stmt, preceding []ast.Stmt, beforeEachAssertions []string) { rangeStmt, ok := stmt.(*ast.RangeStmt) if !ok { return } sel, ok := rangeStmt.X.(*ast.SelectorExpr) if !ok || sel.Sel.Name != "Items" { return } if !bodyContainsExpect(rangeStmt.Body) { return } if hasBeEmptyAssertionBefore(preceding, sel) { return } if slices.Contains(beforeEachAssertions, nodeString(sel)) { return } pass.Report(analysis.Diagnostic{Pos: rangeStmt.Pos(), Message: vacuousPassMessage}) }
checkBlockForVacuousPassthen becomes a loop that callscheckRangeStmt(pass, stmt, body.List[:i], assertions), and the statement path calls it withparent.List[:idx].stmtIndexandcontainsStringbecome unnecessary.🤖 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 `@hack/tools/hypershiftlinter/analyzers/vacuouspass/vacuouspass.go` around lines 94 - 157, Deduplicate the repeated range validation and diagnostic logic by introducing a shared helper, such as checkRangeStmt, that accepts the statement, preceding statements, and beforeEachAssertions. Update checkBlockForVacuousPass and checkStmtForVacuousPass to delegate to it with the appropriate preceding slice, centralize the diagnostic text in one constant, and remove now-unused stmtIndex and containsString usage.hack/tools/hypershiftlinter/analyzers/ipv6url/ipv6url.go (1)
20-20: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBroaden the host-port pattern.
%s:%[dv]misses common IPv6-unsafe forms. It does not match%s:%s,%v:%s, or verbs with flags or explicit argument indexes such as%[1]s:%[2]d.fmt.Sprintf("https://%s:%s/healthz", host, port)is a frequent pattern in E2E code and passes the check today.♻️ Proposed pattern
-var hostPortPattern = regexp.MustCompile(`%s:%[dv]`) +// Matches host:port verb pairs such as %s:%d, %s:%s, %v:%d, and %[1]s:%[2]d. +var hostPortPattern = regexp.MustCompile(`%(?:\[\d+\])?[sv]:%(?:\[\d+\])?[dsv]`)🤖 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 `@hack/tools/hypershiftlinter/analyzers/ipv6url/ipv6url.go` at line 20, Broaden the hostPortPattern regular expression to detect both string and value formatting verbs on each side of the colon, including flags and explicit argument indexes such as %s:%s, %v:%s, and %[1]s:%[2]d. Keep the pattern focused on format placeholders used to construct host-port URLs.hack/tools/hypershiftlinter/analyzers/guestcluster/guestcluster.go (2)
61-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the case variants with the lowercased string.
Lines 63 and 66 mix a lowercased comparison and three case-specific comparisons. A single lowercased
Containscovers every case variant, includingGUESTCLUSTER.♻️ Proposed simplification
func containsGuestCluster(s string) bool { lower := strings.ToLower(s) - if strings.Contains(lower, "guest cluster") { - return true - } - if strings.Contains(s, "guestCluster") || strings.Contains(s, "GuestCluster") || strings.Contains(s, "guestcluster") { - return true - } - return false + return strings.Contains(lower, "guest cluster") || strings.Contains(lower, "guestcluster") }🤖 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 `@hack/tools/hypershiftlinter/analyzers/guestcluster/guestcluster.go` around lines 61 - 70, Update containsGuestCluster to perform the “guest cluster” and “guestcluster” checks against the existing lower variable, removing the separate case-specific strings and preserving detection of all casing variants.
39-56: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSkip import paths when scanning string literals.
ast.Inspectvisits import path literals. An import of a package whose path containsguestclusterproduces a diagnostic that the author cannot fix by renaming text. Skipfile.Importspositions, or inspect declarations other than*ast.ImportSpec.♻️ Proposed guard
ast.Inspect(file, func(n ast.Node) bool { + if _, ok := n.(*ast.ImportSpec); ok { + return false + } lit, ok := n.(*ast.BasicLit) if !ok || lit.Kind != token.STRING { return true }🤖 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 `@hack/tools/hypershiftlinter/analyzers/guestcluster/guestcluster.go` around lines 39 - 56, Update the AST scan around ast.Inspect so string literals belonging to file.Imports or *ast.ImportSpec are excluded before containsGuestCluster is checked. Continue reporting matching non-import string literals with the existing diagnostic behavior.hack/tools/hypershiftlinter/analyzers/pathutil/pathutil.go (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a more specific package name than
pathutil.The coding guidelines state: "Do not use
util,common, or similarly vague package names." The package holds test-classification predicates, so a name such astestpathortestkindstates the purpose and reads better at the call site (testpath.IsV2E2ETest).As per coding guidelines: "Do not use
util,common, or similarly vague package names."🤖 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 `@hack/tools/hypershiftlinter/analyzers/pathutil/pathutil.go` at line 1, Rename the vague pathutil package to a purpose-specific name such as testpath or testkind, reflecting that it contains test-classification predicates. Update the package declaration, directory/package references, and call sites such as IsV2E2ETest so imports and qualified usages remain consistent.Source: Coding guidelines
7-19: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNormalize the path separator before matching.
Both predicates match the literals
test/e2e/,test/integration/, andtest/e2e/v2/. Callers passpass.Fset.File(file.Pos()).Name(), which uses the host separator. On a Windows host every predicate returnsfalse, and all seven analyzers become silent no-ops. CI runs on Linux, so this affects local runs only.♻️ Proposed normalization
-import "strings" +import ( + "path/filepath" + "strings" +) // IsUnitTest returns true for _test.go files that are NOT e2e or integration tests. // TESTING.md conventions apply to unit tests only. func IsUnitTest(filename string) bool { + filename = filepath.ToSlash(filename) if !strings.HasSuffix(filename, "_test.go") { return false } @@ func IsV2E2ETest(filename string) bool { - return strings.Contains(filename, "test/e2e/v2/") + return strings.Contains(filepath.ToSlash(filename), "test/e2e/v2/") }🤖 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 `@hack/tools/hypershiftlinter/analyzers/pathutil/pathutil.go` around lines 7 - 19, Normalize the filename path separators at the start of IsUnitTest and IsV2E2ETest before checking the test/e2e/, test/integration/, and test/e2e/v2/ patterns, using the repository’s existing path-normalization convention if available. Preserve the current suffix and directory-matching behavior after normalization.hack/tools/hypershiftlinter/analyzers/contextbackground/contextbackground.go (1)
35-37: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrecompute the exempt ranges once per file.
isInsideExemptFuncwalks the whole file for everycontext.Background()call, andcontainsNodewalks each argument subtree again. The cost grows with (number of matches × file size). Collect the position ranges ofBeforeSuiteandDeferCleanupclosures in a single pass, then testcall.Pos()against those ranges.♻️ Suggested approach
// collectExemptRanges returns the position ranges of BeforeSuite and // DeferCleanup closures in the file. func collectExemptRanges(file *ast.File) [][2]token.Pos { var ranges [][2]token.Pos ast.Inspect(file, func(n ast.Node) bool { call, ok := n.(*ast.CallExpr) if !ok { return true } switch callName(call) { case "BeforeSuite", "DeferCleanup": ranges = append(ranges, [2]token.Pos{call.Pos(), call.End()}) } return true }) return ranges }Then replace the
isInsideExemptFunc(file, call)check with a range containment test.Also applies to: 62-84
🤖 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 `@hack/tools/hypershiftlinter/analyzers/contextbackground/contextbackground.go` around lines 35 - 37, Precompute exempt ranges once per file instead of calling isInsideExemptFunc for every context.Background() match. Add a helper such as collectExemptRanges that scans BeforeSuite and DeferCleanup calls, store each call’s Pos and End, and update the analyzer to test each call.Pos() against those ranges; remove the repeated containsNode traversal while preserving exemption behavior.
🤖 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 @.github/workflows/test-linter.yaml:
- Around line 8-10: Update the path filters for the linter test workflow to also
trigger when hack/tools/go.mod, hack/tools/go.sum, or Makefile changes, while
retaining the existing hack/tools/hypershiftlinter/** coverage.
In `@hack/tools/hypershiftlinter/analyzers/testcasename/testcasename.go`:
- Line 21: Update the namePattern regular expression to require an uppercase
“When” and a comma between the condition and “it should”; remove the
case-insensitive flag and optional comma so only the exact test-case format is
accepted.
In `@hack/tools/hypershiftlinter/plugin.go`:
- Around line 18-24: Reduce the exported API in the hypershift linter plugin by
renaming Settings, AnalyzerSettings, and AllAnalyzers to unexported identifiers,
then update all references within the plugin accordingly. Preserve
BuildAnalyzers as the only exported symbol required by the plugin command.
In `@Makefile`:
- Around line 105-106: Update the Hypershift linter plugin build rule for
HYPERSHIFTLINTER_PLUGIN to set CGO_ENABLED=1, matching the KUBEAPILINTER_PLUGIN
build configuration while preserving the existing Go plugin build command.
---
Nitpick comments:
In
`@hack/tools/hypershiftlinter/analyzers/contextbackground/contextbackground.go`:
- Around line 35-37: Precompute exempt ranges once per file instead of calling
isInsideExemptFunc for every context.Background() match. Add a helper such as
collectExemptRanges that scans BeforeSuite and DeferCleanup calls, store each
call’s Pos and End, and update the analyzer to test each call.Pos() against
those ranges; remove the repeated containsNode traversal while preserving
exemption behavior.
In `@hack/tools/hypershiftlinter/analyzers/guestcluster/guestcluster.go`:
- Around line 61-70: Update containsGuestCluster to perform the “guest cluster”
and “guestcluster” checks against the existing lower variable, removing the
separate case-specific strings and preserving detection of all casing variants.
- Around line 39-56: Update the AST scan around ast.Inspect so string literals
belonging to file.Imports or *ast.ImportSpec are excluded before
containsGuestCluster is checked. Continue reporting matching non-import string
literals with the existing diagnostic behavior.
In `@hack/tools/hypershiftlinter/analyzers/ipv6url/ipv6url.go`:
- Line 20: Broaden the hostPortPattern regular expression to detect both string
and value formatting verbs on each side of the colon, including flags and
explicit argument indexes such as %s:%s, %v:%s, and %[1]s:%[2]d. Keep the
pattern focused on format placeholders used to construct host-port URLs.
In `@hack/tools/hypershiftlinter/analyzers/pathutil/pathutil.go`:
- Line 1: Rename the vague pathutil package to a purpose-specific name such as
testpath or testkind, reflecting that it contains test-classification
predicates. Update the package declaration, directory/package references, and
call sites such as IsV2E2ETest so imports and qualified usages remain
consistent.
- Around line 7-19: Normalize the filename path separators at the start of
IsUnitTest and IsV2E2ETest before checking the test/e2e/, test/integration/, and
test/e2e/v2/ patterns, using the repository’s existing path-normalization
convention if available. Preserve the current suffix and directory-matching
behavior after normalization.
In `@hack/tools/hypershiftlinter/analyzers/vacuouspass/vacuouspass.go`:
- Around line 68-71: Update walkGinkgoBlock so merged is built in an independent
slice rather than appending directly to beforeEachAssertions; use the already
imported slices helper to copy the inherited assertions before adding
localAssertions, preserving the existing ordering and avoiding shared
backing-array mutations.
- Around line 94-157: Deduplicate the repeated range validation and diagnostic
logic by introducing a shared helper, such as checkRangeStmt, that accepts the
statement, preceding statements, and beforeEachAssertions. Update
checkBlockForVacuousPass and checkStmtForVacuousPass to delegate to it with the
appropriate preceding slice, centralize the diagnostic text in one constant, and
remove now-unused stmtIndex and containsString usage.
In `@hack/tools/hypershiftlinter/plugin.go`:
- Around line 47-54: Preallocate the filtered analyzer slice in the
analyzer-selection loop by creating filtered with capacity
len(s.Analyzers.Enable), while retaining its nil/empty length and existing
append behavior.
🪄 Autofix
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: 321ae8f2-97fb-4850-8db9-694a57f7bcfa
⛔ Files ignored due to path filters (41)
hack/tools/hypershiftlinter/analyzers/contextbackground/testdata/src/test/e2e/v2/bad/bad_test.gois excluded by!**/testdata/**hack/tools/hypershiftlinter/analyzers/contextbackground/testdata/src/test/e2e/v2/good/good_test.gois excluded by!**/testdata/**hack/tools/hypershiftlinter/analyzers/guestcluster/testdata/src/test/e2e/v2/bad/bad.gois excluded by!**/testdata/**hack/tools/hypershiftlinter/analyzers/guestcluster/testdata/src/test/e2e/v2/good/good.gois excluded by!**/testdata/**hack/tools/hypershiftlinter/analyzers/ipv6url/testdata/src/test/e2e/v2/bad/bad.gois excluded by!**/testdata/**hack/tools/hypershiftlinter/analyzers/ipv6url/testdata/src/test/e2e/v2/good/good.gois excluded by!**/testdata/**hack/tools/hypershiftlinter/analyzers/sippyannotation/testdata/src/test/e2e/v2/bad/bad.gois excluded by!**/testdata/**hack/tools/hypershiftlinter/analyzers/sippyannotation/testdata/src/test/e2e/v2/good/good.gois excluded by!**/testdata/**hack/tools/hypershiftlinter/analyzers/testcasename/testdata/src/a/bad/bad_test.gois excluded by!**/testdata/**hack/tools/hypershiftlinter/analyzers/testcasename/testdata/src/a/good/good_test.gois excluded by!**/testdata/**hack/tools/hypershiftlinter/analyzers/testfuncname/testdata/src/a/bad/bad_test.gois excluded by!**/testdata/**hack/tools/hypershiftlinter/analyzers/testfuncname/testdata/src/a/good/good_test.gois excluded by!**/testdata/**hack/tools/hypershiftlinter/analyzers/vacuouspass/testdata/src/test/e2e/v2/bad/bad.gois excluded by!**/testdata/**hack/tools/hypershiftlinter/analyzers/vacuouspass/testdata/src/test/e2e/v2/good/good.gois excluded by!**/testdata/**hack/tools/vendor/golang.org/x/tools/go/analysis/analysistest/analysistest.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/go/analysis/checker/checker.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/go/analysis/checker/print.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/go/analysis/internal/internal.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/analysis/driverutil/fix.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/analysis/driverutil/print.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/analysis/driverutil/readfile.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/analysis/driverutil/url.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/analysis/driverutil/validatefix.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/astutil/free/free.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/diff/diff.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/diff/lcs/common.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/diff/lcs/doc.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/diff/lcs/git.shis excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/diff/lcs/labels.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/diff/lcs/old.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/diff/lcs/sequence.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/diff/merge.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/diff/ndiff.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/diff/unified.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/testenv/exec.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/testenv/testenv.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/testenv/testenv_notunix.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/internal/testenv/testenv_unix.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/txtar/archive.gois excluded by!**/vendor/**hack/tools/vendor/golang.org/x/tools/txtar/fs.gois excluded by!**/vendor/**hack/tools/vendor/modules.txtis excluded by!**/vendor/**
📒 Files selected for processing (23)
.github/workflows/lint-reusable.yaml.github/workflows/test-linter-reusable.yaml.github/workflows/test-linter.yamlMakefilehack/tools/go.modhack/tools/hypershiftlinter/analyzers/contextbackground/contextbackground.gohack/tools/hypershiftlinter/analyzers/contextbackground/contextbackground_test.gohack/tools/hypershiftlinter/analyzers/guestcluster/guestcluster.gohack/tools/hypershiftlinter/analyzers/guestcluster/guestcluster_test.gohack/tools/hypershiftlinter/analyzers/ipv6url/ipv6url.gohack/tools/hypershiftlinter/analyzers/ipv6url/ipv6url_test.gohack/tools/hypershiftlinter/analyzers/pathutil/pathutil.gohack/tools/hypershiftlinter/analyzers/pathutil/pathutil_test.gohack/tools/hypershiftlinter/analyzers/sippyannotation/sippyannotation.gohack/tools/hypershiftlinter/analyzers/sippyannotation/sippyannotation_test.gohack/tools/hypershiftlinter/analyzers/testcasename/testcasename.gohack/tools/hypershiftlinter/analyzers/testcasename/testcasename_test.gohack/tools/hypershiftlinter/analyzers/testfuncname/testfuncname.gohack/tools/hypershiftlinter/analyzers/testfuncname/testfuncname_test.gohack/tools/hypershiftlinter/analyzers/vacuouspass/vacuouspass.gohack/tools/hypershiftlinter/analyzers/vacuouspass/vacuouspass_test.gohack/tools/hypershiftlinter/cmd/plugin/main.gohack/tools/hypershiftlinter/plugin.go
| paths: | ||
| - 'hack/tools/hypershiftlinter/**' | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Run linter tests when their module or target changes.
The path filter excludes hack/tools/go.mod, hack/tools/go.sum, and Makefile. A change to any of these files can break make test-linter without running this check.
Proposed fix
paths:
- 'hack/tools/hypershiftlinter/**'
+ - 'hack/tools/go.mod'
+ - 'hack/tools/go.sum'
+ - 'Makefile'As per coding guidelines, “Unit test any code changes and additions.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| paths: | |
| - 'hack/tools/hypershiftlinter/**' | |
| paths: | |
| - 'hack/tools/hypershiftlinter/**' | |
| - 'hack/tools/go.mod' | |
| - 'hack/tools/go.sum' | |
| - 'Makefile' |
🤖 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 @.github/workflows/test-linter.yaml around lines 8 - 10, Update the path
filters for the linter test workflow to also trigger when hack/tools/go.mod,
hack/tools/go.sum, or Makefile changes, while retaining the existing
hack/tools/hypershiftlinter/** coverage.
Source: Coding guidelines
| Run: run, | ||
| } | ||
|
|
||
| var namePattern = regexp.MustCompile(`(?i)^when .+,? it should .+$`) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require the exact test-case format.
The (?i) flag accepts a lowercase when. The optional comma accepts When condition it should result. Both forms violate the required format.
Proposed fix
-var namePattern = regexp.MustCompile(`(?i)^when .+,? it should .+$`)
+var namePattern = regexp.MustCompile(`^When .+, it should .+$`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var namePattern = regexp.MustCompile(`(?i)^when .+,? it should .+$`) | |
| var namePattern = regexp.MustCompile(`^When .+, it should .+$`) |
🤖 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 `@hack/tools/hypershiftlinter/analyzers/testcasename/testcasename.go` at line
21, Update the namePattern regular expression to require an uppercase “When” and
a comma between the condition and “it should”; remove the case-insensitive flag
and optional comma so only the exact test-case format is accepted.
Source: Coding guidelines
| type Settings struct { | ||
| Analyzers *AnalyzerSettings `json:"analyzers"` | ||
| } | ||
|
|
||
| type AnalyzerSettings struct { | ||
| Enable []string `json:"enable"` | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Reduce the exported API surface.
Only BuildAnalyzers is required by hack/tools/hypershiftlinter/cmd/plugin/main.go. Make Settings, AnalyzerSettings, and AllAnalyzers unexported.
As per coding guidelines, “Do not expose unnecessary symbols (keep exported API minimal).”
Also applies to: 58-68
🤖 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 `@hack/tools/hypershiftlinter/plugin.go` around lines 18 - 24, Reduce the
exported API in the hypershift linter plugin by renaming Settings,
AnalyzerSettings, and AllAnalyzers to unexported identifiers, then update all
references within the plugin accordingly. Preserve BuildAnalyzers as the only
exported symbol required by the plugin command.
Source: Coding guidelines
| $(HYPERSHIFTLINTER_PLUGIN): $(TOOLS_DIR)/go.mod $(HYPERSHIFTLINTER_SRC) # Build hypershiftlinter as Go plugin | ||
| cd $(TOOLS_DIR); $(GO) build -a -buildmode=plugin -o $(HYPERSHIFTLINTER_PLUGIN) ./hypershiftlinter/cmd/plugin |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 'KUBEAPILINTER_PLUGIN|HYPERSHIFTLINTER_PLUGIN|CGO_ENABLED|buildmode=plugin' MakefileRepository: openshift/hypershift
Length of output: 3244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Makefile context ---'
sed -n '45,60p;90,145p' Makefile
printf '%s\n' '--- CI references to lint targets and CGO_ENABLED ---'
rg -n -C 3 '(^|[[:space:]])(make|$(MAKE)).*(lint|hypershift-lint)|CGO_ENABLED|test-linter|verify-parallel' .github Makefile 2>/dev/null || true
printf '%s\n' '--- Local Go build-mode documentation ---'
if command -v go >/dev/null 2>&1; then
go version
go help buildmode | sed -n '1,180p'
else
printf '%s\n' 'go is not available'
fiRepository: openshift/hypershift
Length of output: 11330
🌐 Web query:
Official Go documentation for whether -buildmode=plugin requires CGO_ENABLED=1
💡 Result:
The Go toolchain does not require CGO_ENABLED=1 for all operations. Whether it is required depends entirely on your specific project and build environment. CGO_ENABLED=1 is the default setting for native builds on systems where cgo is expected to work [1][2]. You must set CGO_ENABLED=1 if your project requires cgo features, such as: 1. Importing the "C" pseudo-package to call C code from Go [1][2]. 2. Utilizing certain Go standard library features that rely on host-specific C libraries for behavior (e.g., specific DNS resolution or NSSwitch lookups on Linux) [3]. If your project does not import "C" and you do not require host-specific C functionality, you can build with CGO_ENABLED=0 [1][2]. Setting CGO_ENABLED=0 is commonly used to create statically linked, standalone binaries that do not have dependencies on dynamic C libraries [3]. Key points regarding the environment variable: - Default Behavior: CGO_ENABLED is enabled by default for native builds where a C compiler (like gcc or clang) is detected on the system PATH [1][2]. It is disabled by default during cross-compilation [1][2]. - Build Constraints: If cgo is disabled (CGO_ENABLED=0), any Go files that import "C" will be excluded from the build because the "cgo" build constraint will not be satisfied [1][2]. - Requirements: When CGO_ENABLED=1 is used, a C compiler is required [2][4]. For example, Go 1.25+ requires a C compiler with DWARF 5 support on Windows, and general cgo usage requires gcc 4.6 or newer [5].
Citations:
- 1: https://pkg.go.dev/cmd/cgo
- 2: https://go.dev/src/cmd/cgo/doc.go
- 3: https://stackoverflow.com/questions/64531437/why-is-cgo-enabled-1-default
- 4: https://go.dev/doc/install/source
- 5: https://tip.golang.org/wiki/MinimumRequirements
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if ! command -v go >/dev/null 2>&1; then
printf '%s\n' 'go is not available'
exit 0
fi
GOROOT="$(go env GOROOT)"
printf '%s\n' '--- Go tool source: plugin and CGO checks ---'
rg -n -C 5 'buildmode=plugin|Buildmode.*plugin|CgoEnabled|cgo.*plugin|plugin.*cgo' \
"$GOROOT/src/cmd/go/internal" "$GOROOT/src/cmd/link/internal" 2>/dev/null | head -240
printf '%s\n' '--- Hypershift linter package imports ---'
rg -n -C 2 '(^|[[:space:]])import[[:space:]]|\"C\"|plugin' hack/tools/hypershiftlinter hack/tools/go.mod 2>/dev/null | head -240Repository: openshift/hypershift
Length of output: 32095
🏁 Script executed:
#!/bin/bash
set -euo pipefail
GOROOT="$(go env GOROOT)"
sed -n '175,205p;285,315p' "$GOROOT/src/cmd/go/internal/work/init.go"Repository: openshift/hypershift
Length of output: 1717
🏁 Script executed:
#!/bin/bash
set -euo pipefail
GOROOT="$(go env GOROOT)"
printf '%s\n' '--- External-linking decision ---'
rg -n -C 12 'func externalLinkingReason|externalLinkingReason\\(' \
"$GOROOT/src/cmd/go/internal/load" "$GOROOT/src/cmd/go/internal/work" 2>/dev/null | head -260
printf '%s\n' '--- Plugin build-mode references in Go tool source ---'
rg -n -C 8 'BuildBuildmode == "plugin"|BuildBuildmode != "plugin"|buildmode=plugin' \
"$GOROOT/src/cmd/go/internal" 2>/dev/null | head -260Repository: openshift/hypershift
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
GOROOT="$(go env GOROOT)"
printf '%s\n' '--- Definitions and calls ---'
rg -n -C 8 'externalLinkingReason' \
"$GOROOT/src/cmd/go/internal/load" \
"$GOROOT/src/cmd/go/internal/work" || true
printf '%s\n' '--- Build-mode plugin references ---'
rg -n -C 8 'buildmode=plugin|BuildBuildmode.*plugin' \
"$GOROOT/src/cmd/go/internal" || trueRepository: openshift/hypershift
Length of output: 9155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
GOROOT="$(go env GOROOT)"
sed -n '2674,2725p' "$GOROOT/src/cmd/go/internal/load/pkg.go"Repository: openshift/hypershift
Length of output: 1934
Set CGO_ENABLED=1 for the Hypershift linter plugin build.
When CGO_ENABLED=0, Go rejects -buildmode=plugin because it requires external CGO linking. Match KUBEAPILINTER_PLUGIN so make lint can build the plugin.
🤖 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 `@Makefile` around lines 105 - 106, Update the Hypershift linter plugin build
rule for HYPERSHIFTLINTER_PLUGIN to set CGO_ENABLED=1, matching the
KUBEAPILINTER_PLUGIN build configuration while preserving the existing Go plugin
build command.
Remove hypershiftlinter from `make lint` and `.golangci.yml` custom registration — the plugin framework and analyzers are introduced here, but enabling them in lint and fixing existing violations belongs in the follow-up PR (openshift#9271). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
What this PR does / why we need it:
Enables the hypershiftlinter golangci-lint plugin (added in #9237) and fixes all existing test naming violations across the codebase to match TESTING.md conventions (
When ..., it should ...).Also fixes
make test-changedto exclude theapi/submodule (which has its owngo.modand cannot be listed from the root module).Which issue(s) this PR fixes:
Fixes CNTRLPLANE-4008
Special notes for your reviewer:
Depends on #9237 merging first so the
hypershiftlinter.soplugin binary exists.Checklist:
Summary by CodeRabbit
Chores
Tests