STOR-2996: Run unit-tests in /legacy subdir

[mpatlasov]

The initial PR moving gcp-pd-csi-driver-operator from https://github.com/openshift/gcp-pd-csi-driver-operator to /legacy subdir if csi-operator missed the issue: CI unit-test job simply executes make test-unit, and it does not cross module boundaries (because legacy/gcp-pd-csi-driver-operator/ has its own go.mod)

https://redhat.atlassian.net/browse/STOR-2996

[coderabbitai]

📝 Walkthrough

Walkthrough

The Makefile now adds a test-unit-legacy target that runs make test-unit in legacy/gcp-pd-csi-driver-operator. The existing test-unit target now depends on test-unit-legacy, and test-unit-legacy is marked as .PHONY. The change is limited to build target wiring.

Changes

File	Change Summary
Makefile	Added test-unit-legacy; made test-unit depend on it; declared test-unit-legacy as .PHONY

Sequence Diagram(s)

Not applicable; the change is a build-target delegation with no runtime interaction flow.

Estimated code review effort: Low

Related issues: None provided.

Related PRs: None provided.

Suggested labels: build, makefile

Suggested reviewers: None specified.

Poem

A Makefile learns an older tune,
It calls the legacy tests right soon,
One target leads where old paths run,
Then hands the work back when done.

🚥 Pre-merge checks | ✅ 15

✅ Passed checks (15 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	Only Makefile changed; no Ginkgo test titles or test files were added or modified, so the naming rule is unaffected.
Test Structure And Quality	✅ Passed	Only Makefile changed; no Ginkgo test code was added or modified, so the test-quality check is not applicable.
Microshift Test Compatibility	✅ Passed	Only Makefile targets changed; no new Ginkgo tests or MicroShift-sensitive APIs were added.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	Only Makefile changed; no new Ginkgo tests or SNO-relevant e2e code was added.
Topology-Aware Scheduling Compatibility	✅ Passed	Only Makefile changed; no deployment manifests, operator code, or controllers were modified, so topology-aware scheduling checks are not applicable.
Ote Binary Stdout Contract	✅ Passed	Only Makefile changed; no process-level code or stdout writes were introduced.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	Only Makefile changed; no new Ginkgo e2e tests or network-sensitive test code were added, so this check is not applicable.
No-Weak-Crypto	✅ Passed	Only Makefile targets were added; no crypto code, weak algorithms, custom crypto, or secret comparisons were introduced.
Container-Privileges	✅ Passed	PR only changes Makefile; no container/K8s manifests or privilege-related settings are introduced or modified.
No-Sensitive-Data-In-Logs	✅ Passed	Only Makefile targets were added; no logging statements or sensitive data exposure were introduced.
Title check	✅ Passed	The title clearly matches the main change: adding legacy subdir unit-test execution.
Description check	✅ Passed	The description is directly related, explaining why unit tests need to run under the legacy module.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

legacy/gcp-pd-csi-driver-operator/pkg/operator/starter_test.go (1)

76-82: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

wantErr: true contradicts actual withCustomLabels behavior for this test case.

createInfraCR: true means the Infrastructure object is added to the lister, so infraLister.Get(globalInfrastructureName) succeeds and withCustomLabels returns nil (it only errors when the lister lookup fails — see starter.go lines 279-309: "withCustomLabels" returns error only when "infraLister.Get(globalInfrastructureName)" fails; otherwise it builds/appends "--extra-labels" and returns nil. Empty ResourceLabels is not an error path, it just falls back to the default label.

With wantErr: true but an actual nil error, this test case will now fail on every run ((err != nil) != test.wantErr → false != true). If the intent is to prove the top-level Makefile never runs legacy unit tests, deliberately breaking a test to demonstrate that is confusing and risky if this ever gets merged as-is — it leaves the suite in a permanently broken state for anyone who does run legacy/ tests.

🐛 Proposed fix (revert to correct expectation)

 		{
 			name:          "labels not configured",
 			labels:        []v1.GCPResourceLabel{},
 			expArgList:    fmt.Sprintf("--extra-labels=%s", fmt.Sprintf(ocpDefaultLabelFmt, infraObj.Status.InfrastructureName)),
 			createInfraCR: true,
-			wantErr:       true,
+			wantErr:       false,
 		},

🤖 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 `@legacy/gcp-pd-csi-driver-operator/pkg/operator/starter_test.go` around lines
76 - 82, The “labels not configured” case in starter_test.go has the wrong
expectation for withCustomLabels: when createInfraCR is true,
infraLister.Get(globalInfrastructureName) succeeds and empty labels should still
fall back to the default extra-labels value without returning an error. Update
this test case to reflect the actual behavior of withCustomLabels in starter.go
by keeping the expected argument list and changing wantErr to the correct value
so the test matches the non-error path.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@legacy/gcp-pd-csi-driver-operator/pkg/operator/starter_test.go`:
- Around line 76-82: The “labels not configured” case in starter_test.go has the
wrong expectation for withCustomLabels: when createInfraCR is true,
infraLister.Get(globalInfrastructureName) succeeds and empty labels should still
fall back to the default extra-labels value without returning an error. Update
this test case to reflect the actual behavior of withCustomLabels in starter.go
by keeping the expected argument list and changing wantErr to the correct value
so the test matches the non-error path.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 33e2debe-f99b-420c-9a3a-1caf3e0f86eb

📥 Commits

Reviewing files that changed from the base of the PR and between 2fc1e7d and 175606d.

📒 Files selected for processing (1)

• legacy/gcp-pd-csi-driver-operator/pkg/operator/starter_test.go

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mpatlasov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [mpatlasov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Makefile`:
- Around line 20-25: The top-level test-unit target is missing a phony
declaration, so make may treat an existing file or directory named test-unit as
up to date and skip running the recipe. Update the Makefile’s phony target list
to include test-unit alongside test-unit-legacy, keeping the existing
test-unit-legacy dependency and recipe unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 8bf71414-c140-42be-9531-bedc1d417a6a

📥 Commits

Reviewing files that changed from the base of the PR and between 08b3278 and a8f7cf5.

📒 Files selected for processing (1)

• Makefile

[openshift-ci-robot]

@mpatlasov: This pull request references STOR-1855 which is a valid jira issue.

Details

In response to this:

Let's firstly verify an assumption: top-level Makefile never executes unit-tests in legacy/

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.

[mpatlasov]

/assign @jsafrane

[openshift-ci-robot]

@mpatlasov: This pull request references STOR-2996 which is a valid jira issue.

Details

In response to this:

The initial PR moving gcp-pd-csi-driver-operator from https://github.com/openshift/gcp-pd-csi-driver-operator to /legacy subdir if csi-operator missed the issue: CI unit-test job simply executes make test-unit, and it does not cross module boundaries (because legacy/gcp-pd-csi-driver-operator/ has its own go.mod)

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.

[dobsonj]

/lgtm
/retest

[mpatlasov]

/retest

[openshift-ci]

@mpatlasov: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[mpatlasov]

/verified by ci

[openshift-ci-robot]

@mpatlasov: This PR has been marked as verified by ci.

Details

In response to this:

/verified by ci

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.