STOR-2997: Minimal implementation of GCP PD CSI driver operator#576
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@mpatlasov: This pull request references STOR-2997 which is a valid jira issue. 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. |
|
@mpatlasov: GitHub didn't allow me to request PR reviews from the following users: openshift/storage. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a GCP PD CSI Driver Operator CLI entrypoint with Cobra-based root and Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/driver/gcp-pd/gcp_pd.go (1)
36-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix malformed log call and redundant/non-idiomatic error message.
klog.Error(nil, "...")treatsnilas afmt.Print-style argument (klog'sErrorargs are "handled in the manner offmt.Print"), so this prints something like<nil>Flavour HyperShift is not supported!instead of a clean message. Additionally, this error is already logged again by the caller (RunOperatorlogs"error building operator config: %v"on any non-nil error from this builder), so the localklog.Errorcall is redundant. Thefmt.Errorfmessage is also capitalized and ends with!, which violates Go's error-string convention (lower-case, no trailing punctuation).🔧 Proposed fix
if flavour != generator.FlavourStandalone { - klog.Error(nil, "Flavour HyperShift is not supported!") - return nil, fmt.Errorf("Flavour HyperShift is not supported!") + return nil, fmt.Errorf("flavour %q is not supported", flavour) }🤖 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/driver/gcp-pd/gcp_pd.go` around lines 36 - 39, The gcp_pd builder’s unsupported-flavour branch is logging incorrectly and returning a non-idiomatic error. In the config builder path in `gcp_pd.go`, remove the local `klog.Error(nil, ...)` call because `RunOperator` already logs builder failures, and change the returned `fmt.Errorf` message to a lowercase, punctuation-free error string. Keep the check on `flavour != generator.FlavourStandalone` and ensure the branch only returns the clean error.cmd/gcp-pd-csi-driver-operator/main.go (1)
27-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIgnored error return from
cmd.Help().
cmd.Help()returns anerrorthat's discarded here.🔧 Proposed fix
Run: func(cmd *cobra.Command, args []string) { - cmd.Help() + if err := cmd.Help(); err != nil { + klog.Errorf("failed to print help: %v", err) + } os.Exit(1) },As per path instructions, "Never ignore error returns" for
**/*.go.🤖 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/gcp-pd-csi-driver-operator/main.go` around lines 27 - 30, The cobra command’s Run handler is discarding the error from cmd.Help(), which violates the “never ignore error returns” rule. Update the Run function in main.go to check and handle the return value from cmd.Help() explicitly, using the cmd.Help() call itself as the unique locator, and ensure the process still exits with a non-zero status if Help fails.Source: Path instructions
🤖 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.
Nitpick comments:
In `@cmd/gcp-pd-csi-driver-operator/main.go`:
- Around line 27-30: The cobra command’s Run handler is discarding the error
from cmd.Help(), which violates the “never ignore error returns” rule. Update
the Run function in main.go to check and handle the return value from cmd.Help()
explicitly, using the cmd.Help() call itself as the unique locator, and ensure
the process still exits with a non-zero status if Help fails.
In `@pkg/driver/gcp-pd/gcp_pd.go`:
- Around line 36-39: The gcp_pd builder’s unsupported-flavour branch is logging
incorrectly and returning a non-idiomatic error. In the config builder path in
`gcp_pd.go`, remove the local `klog.Error(nil, ...)` call because `RunOperator`
already logs builder failures, and change the returned `fmt.Errorf` message to a
lowercase, punctuation-free error string. Keep the check on `flavour !=
generator.FlavourStandalone` and ensure the branch only returns the clean error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 51202570-b405-4a12-9cee-b72111c8a207
⛔ Files ignored due to path filters (30)
assets/overlays/gcp-pd/generated/standalone/cabundle_cm.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller_pdb.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller_sa.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/csidriver.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/kube_rbac_proxy_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/kube_rbac_proxy_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/lease_leader_election_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/lease_leader_election_rolebinding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_attacher_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_provisioner_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_resizer_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_snapshotter_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/manifests.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/node.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/node_privileged_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/node_sa.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/privileged_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/prometheus_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/prometheus_rolebinding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/service.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/servicemonitor.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/storageclass.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/storageclass_reader_resizer_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/storageclass_ssd.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumeattributesclass_reader_provisioner_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumeattributesclass_reader_resizer_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumesnapshot_reader_provisioner_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumesnapshotclass.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumesnapshotclass_images.yamlis excluded by!**/generated/**
📒 Files selected for processing (2)
cmd/gcp-pd-csi-driver-operator/main.gopkg/driver/gcp-pd/gcp_pd.go
|
/retest |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
|
/test hypershift-e2e-aks |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Dockerfile.gcp-pd (2)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid copying entire build context.
COPY . .copies the full repository into the builder stage instead of specific files/directories needed for the build. As per path instructions, "COPY specific files, not entire context."🤖 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 `@Dockerfile.gcp-pd` at line 3, Replace the broad COPY instruction in the Docker build stage with explicit COPY directives for only the files and directories required by the build, excluding the rest of the repository context.Source: Path instructions
6-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing
HEALTHCHECKinstruction.No
HEALTHCHECKis defined for the final image. As per path instructions, "HEALTHCHECK defined."🤖 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 `@Dockerfile.gcp-pd` around lines 6 - 10, Add a HEALTHCHECK instruction to the final image definition in Dockerfile.gcp-pd, alongside the existing ENTRYPOINT and LABEL directives. Configure it to invoke an appropriate health probe for gcp-pd-csi-driver-operator and preserve the current startup command.Source: Path instructions
🤖 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 `@Dockerfile.gcp-pd`:
- Around line 6-10: Update the final image stage in the Dockerfile around the
gcp-pd-csi-driver-operator ENTRYPOINT to add a non-root USER directive, ensuring
the operator runs as non-root while preserving the existing image metadata and
startup command.
---
Nitpick comments:
In `@Dockerfile.gcp-pd`:
- Line 3: Replace the broad COPY instruction in the Docker build stage with
explicit COPY directives for only the files and directories required by the
build, excluding the rest of the repository context.
- Around line 6-10: Add a HEALTHCHECK instruction to the final image definition
in Dockerfile.gcp-pd, alongside the existing ENTRYPOINT and LABEL directives.
Configure it to invoke an appropriate health probe for
gcp-pd-csi-driver-operator and preserve the current startup command.
🪄 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: 19e8acdf-4877-4e40-93b2-802e0864bd31
📒 Files selected for processing (1)
Dockerfile.gcp-pd
| FROM registry.ci.openshift.org/ocp/5.0:base-rhel9 | ||
| COPY --from=builder /go/src/github.com/openshift/gcp-pd-csi-driver-operator/gcp-pd-csi-driver-operator /usr/bin/ | ||
| COPY --from=builder /go/src/github.com/openshift/csi-operator/bin/gcp-pd-csi-driver-operator /usr/bin/ | ||
| ENTRYPOINT ["/usr/bin/gcp-pd-csi-driver-operator"] | ||
| LABEL io.k8s.display-name="OpenShift GCP PD CSI Driver Operator" \ | ||
| io.k8s.description="The GCP PD CSI Driver Operator installs and maintains the GCP PD CSI Driver on a cluster." |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing non-root USER directive in final image.
The final stage runs the operator without setting a non-root USER, so the container defaults to root. As per path instructions, "USER non-root; never run as root."
🔒 Proposed fix
FROM registry.ci.openshift.org/ocp/5.0:base-rhel9
COPY --from=builder /go/src/github.com/openshift/csi-operator/bin/gcp-pd-csi-driver-operator /usr/bin/
+USER 65532:65532
ENTRYPOINT ["/usr/bin/gcp-pd-csi-driver-operator"]📝 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.
| FROM registry.ci.openshift.org/ocp/5.0:base-rhel9 | |
| COPY --from=builder /go/src/github.com/openshift/gcp-pd-csi-driver-operator/gcp-pd-csi-driver-operator /usr/bin/ | |
| COPY --from=builder /go/src/github.com/openshift/csi-operator/bin/gcp-pd-csi-driver-operator /usr/bin/ | |
| ENTRYPOINT ["/usr/bin/gcp-pd-csi-driver-operator"] | |
| LABEL io.k8s.display-name="OpenShift GCP PD CSI Driver Operator" \ | |
| io.k8s.description="The GCP PD CSI Driver Operator installs and maintains the GCP PD CSI Driver on a cluster." | |
| FROM registry.ci.openshift.org/ocp/5.0:base-rhel9 | |
| COPY --from=builder /go/src/github.com/openshift/csi-operator/bin/gcp-pd-csi-driver-operator /usr/bin/ | |
| USER 65532:65532 | |
| ENTRYPOINT ["/usr/bin/gcp-pd-csi-driver-operator"] | |
| LABEL io.k8s.display-name="OpenShift GCP PD CSI Driver Operator" \ | |
| io.k8s.description="The GCP PD CSI Driver Operator installs and maintains the GCP PD CSI Driver on a cluster." |
🤖 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 `@Dockerfile.gcp-pd` around lines 6 - 10, Update the final image stage in the
Dockerfile around the gcp-pd-csi-driver-operator ENTRYPOINT to add a non-root
USER directive, ensuring the operator runs as non-root while preserving the
existing image metadata and startup command.
Source: Path instructions
mpatlasov
force-pushed
the
STOR-2997-Implement-GCP-PD-CSI-driver-operator-inside-csi-operator-framework
branch
from
fa76b6a to
d85ac06
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cmd/gcp-pd-csi-driver-operator/main.go`:
- Line 28: Update the Cobra command handler around cmd.Help() to handle its
returned error instead of discarding it. Use the error-returning command flow,
such as RunE, and return cmd.Help() so help-output failures propagate; ensure no
Go error return remains ignored.
In `@Dockerfile.gcp-pd`:
- Line 3: Replace the broad COPY . . instruction in the Dockerfile build stage
with explicit COPY directives for only the manifests and source directories
required by this build target. Exclude secrets, credentials, build artifacts,
and unrelated repository files from the builder context.
In `@pkg/driver/gcp-pd/gcp_pd.go`:
- Line 38: Update the error string returned by the HyperShift handling in the
relevant function to remove the trailing exclamation mark, preserving the exact
message text as “Flavour HyperShift is not supported”.
🪄 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: b1bd05f7-b7fc-430b-877a-41c866600a9b
⛔ Files ignored due to path filters (32)
assets/overlays/gcp-pd/generated/standalone/cabundle_cm.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller_hostnetwork_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller_pdb.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller_sa.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/csidriver.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/hostnetwork_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/kube_rbac_proxy_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/kube_rbac_proxy_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/lease_leader_election_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/lease_leader_election_rolebinding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_attacher_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_provisioner_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_resizer_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_snapshotter_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/manifests.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/node.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/node_privileged_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/node_sa.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/privileged_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/prometheus_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/prometheus_rolebinding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/service.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/servicemonitor.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/storageclass.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/storageclass_reader_resizer_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/storageclass_ssd.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumeattributesclass_reader_provisioner_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumeattributesclass_reader_resizer_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumesnapshot_reader_provisioner_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumesnapshotclass.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumesnapshotclass_images.yamlis excluded by!**/generated/**
📒 Files selected for processing (3)
Dockerfile.gcp-pdcmd/gcp-pd-csi-driver-operator/main.gopkg/driver/gcp-pd/gcp_pd.go
| Use: "gcp-pd-csi-driver-operator", | ||
| Short: "OpenShift GCP PD CSI Driver Operator", | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| cmd.Help() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle the cmd.Help() error.
Line 28 discards Cobra’s returned error, so help-output failures are silently lost. Check and propagate/handle it, for example by using RunE and returning cmd.Help().
As per path instructions: Go code must never ignore error returns.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 28-28: Error return value of cmd.Help is not checked
(errcheck)
🤖 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/gcp-pd-csi-driver-operator/main.go` at line 28, Update the Cobra command
handler around cmd.Help() to handle its returned error instead of discarding it.
Use the error-returning command flow, such as RunE, and return cmd.Help() so
help-output failures propagate; ensure no Go error return remains ignored.
Sources: Path instructions, Linters/SAST tools
| COPY legacy/gcp-pd-csi-driver-operator . | ||
| RUN make | ||
| WORKDIR /go/src/github.com/openshift/csi-operator | ||
| COPY . . |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not copy the entire build context.
COPY . . places every repository file into the builder stage, potentially including secrets, credentials, and unrelated artifacts. Copy only the manifests and source directories required by this build target.
As per path instructions, “COPY specific files, not entire context.”
🤖 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 `@Dockerfile.gcp-pd` at line 3, Replace the broad COPY . . instruction in the
Dockerfile build stage with explicit COPY directives for only the manifests and
source directories required by this build target. Exclude secrets, credentials,
build artifacts, and unrelated repository files from the builder context.
Source: Path instructions
mpatlasov
force-pushed
the
STOR-2997-Implement-GCP-PD-CSI-driver-operator-inside-csi-operator-framework
branch
from
d85ac06 to
3c7108f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/driver/gcp-pd/gcp_pd.go`:
- Line 38: Update the unsupported-flavour error in the relevant driver function
to start with a lowercase letter, while retaining the flavour-specific context
by formatting the flavour value in the message.
🪄 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: 926fa496-de4b-4f51-98ab-16491ea1ea52
⛔ Files ignored due to path filters (32)
assets/overlays/gcp-pd/generated/standalone/cabundle_cm.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller_hostnetwork_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller_pdb.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller_sa.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/csidriver.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/hostnetwork_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/kube_rbac_proxy_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/kube_rbac_proxy_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/lease_leader_election_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/lease_leader_election_rolebinding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_attacher_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_provisioner_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_resizer_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_snapshotter_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/manifests.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/node.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/node_privileged_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/node_sa.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/privileged_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/prometheus_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/prometheus_rolebinding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/service.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/servicemonitor.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/storageclass.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/storageclass_reader_resizer_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/storageclass_ssd.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumeattributesclass_reader_provisioner_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumeattributesclass_reader_resizer_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumesnapshot_reader_provisioner_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumesnapshotclass.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumesnapshotclass_images.yamlis excluded by!**/generated/**
📒 Files selected for processing (3)
Dockerfile.gcp-pdcmd/gcp-pd-csi-driver-operator/main.gopkg/driver/gcp-pd/gcp_pd.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/gcp-pd-csi-driver-operator/main.go
- Dockerfile.gcp-pd
| func GetGCPPDOperatorControllerConfig(ctx context.Context, flavour generator.ClusterFlavour, c *clients.Clients) (*config.OperatorControllerConfig, error) { | ||
| if flavour != generator.FlavourStandalone { | ||
| klog.Error(nil, "Flavour HyperShift is not supported") | ||
| return nil, fmt.Errorf("Flavour HyperShift is not supported") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Lowercase the returned error string.
ST1005 still flags this line because Go error strings must not start with an uppercase letter. Use a lowercase, flavour-specific message such as fmt.Errorf("flavour %v is not supported", flavour).
Static analysis reports ST1005: error strings should not be capitalized.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 38-38: ST1005: error strings should not be capitalized
(staticcheck)
🤖 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/driver/gcp-pd/gcp_pd.go` at line 38, Update the unsupported-flavour error
in the relevant driver function to start with a lowercase letter, while
retaining the flavour-specific context by formatting the flavour value in the
message.
Source: Linters/SAST tools
|
/retest-required |
| const ( | ||
| generatedAssetBase = "overlays/gcp-pd/generated" | ||
| ) | ||
|
|
There was a problem hiding this comment.
I see we have the generated assets but there is no generator.CSIDriverGeneratorConfig function, these should be generated from this function, right? Or am I missing something?
|
PR seems somewhat incomplete - when I delete This is because Also, there's a typo in the binary in PR description: I was thinking that this PR could be an intermediate step where you just intend to demonstrate a working binary - however the switch from legacy directory in Containerfile would indicate that the migration to generated files is finished. When I worked on AWS migration I implemented this in a single PR, not saying it's the only right way but I think that first we need to get the generators working and see CI tests are passing with those assets. Then cross-check the assets with the legacy ones - it's easy to misss something. |
|
@RomanBednar , @rhrmo , thank you for review, I fixed a typo in PR description -- nice catch! You are right, the PR is intermediate. It provides the operator binary that works in manual tests and passes CI, but based on pre-defined assets. The next step is to auto-generate assets with generator machinery. SMB operator went through the same process: compare #205 vs. #217.
I'd prefer to make this switch as early as possible to see CI failures (and any other complains) earlier and have time to fix them before the end of the cycle. Let me know if you prefer other approach -- keep the PR open until everything is done and polished before the merge. |
|
/test e2e-azure |
1 similar comment
|
/test e2e-azure |
This PR implements the minimal set of go-files and yaml-assets to enable the command ``` ./bin/gcp-pd-csi-driver-smb-operator start --kubeconfig $KUBECONFIG --namespace openshift-cluster-csi-drivers ``` to install and run GCP PD CSI driver: ``` gcp-pd-csi-driver-controller-c949b748-xkljz 10/10 Running 0 3m2s gcp-pd-csi-driver-controller-c949b748-xvgl9 10/10 Running 0 3m2s gcp-pd-csi-driver-node-5kcw5 3/3 Running 0 3m2s gcp-pd-csi-driver-node-frl74 3/3 Running 0 3m2s gcp-pd-csi-driver-node-gf9k7 3/3 Running 0 3m2s gcp-pd-csi-driver-node-p95sv 3/3 Running 0 3m2s gcp-pd-csi-driver-node-q4r98 3/3 Running 0 3m2s gcp-pd-csi-driver-node-xvmgq 3/3 Running 0 3m2s ```
Use gcp-pd go files from cmd/ and pkg/ dirs rather than legacy/
Previous versions of the operator create one single priv ClusterRole (`gcp-pd-privileged-role`) and two ClusterRoleBindings for that (`gcp-pd-controller-privileged-binding` and `gcp-pd-node-privileged-binding`). With this PR merged, the operator creates another ClusterRole specifically for the controller (`gcp-pd-hostnetwork-role`) and another ClusterRoleBindings for that (`gcp-pd-controller-hostnetwork-binding`). This makes older binding for the controller redundant, this commit deletes it unconditionally.
GCP Dedicated only supports hyperdisk-balanced disk type. Add a new StorageClass asset that will be used instead of standard-csi and ssd-csi when the operator detects GCP Dedicated. At operator startup, fetch the Infrastructure CR (with retry for up to 1 minute during early cluster bootstrap) and check if the GCP region starts with "u-". If so, the cluster runs on GCP Dedicated where only hyperdisk-balanced disks are supported — create only the hyperdisk-balanced StorageClass as default. On regular GCP, the existing standard-csi and ssd-csi StorageClasses are used as before. See corresponding commits for legacy/: 7a3e209 and 7f08260.
mpatlasov
force-pushed
the
STOR-2997-Implement-GCP-PD-CSI-driver-operator-inside-csi-operator-framework
branch
from
3c7108f to
ddb0a38
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/driver/gcp-pd/gcp_pd.go (1)
51-55: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winError string still capitalized (ST1005), plus
klog.Error(nil, ...)misuse.Two issues here:
fmt.Errorf("Flavour HyperShift is not supported")still starts with an uppercase letter — the earlierST1005capitalization review comment on this line was not addressed (only the trailing punctuation was fixed).klog.Error(nil, "...")is not the structuredErrorS(err error, msg string, ...)call —klog.Erroris anilas the first positional argument produces a garbled log line (e.g.<nil>Flavour HyperShift is not supported) instead of a clean message. This log call is also redundant: the caller inpkg/operator/starter.go(RunOperator) already logsklog.Errorf("error building operator config: %v", err)on the returned error.🐛 Proposed fix
if flavour != generator.FlavourStandalone { - klog.Error(nil, "Flavour HyperShift is not supported") - return nil, fmt.Errorf("Flavour HyperShift is not supported") + return nil, fmt.Errorf("flavour %v is not supported", flavour) }🤖 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/driver/gcp-pd/gcp_pd.go` around lines 51 - 55, Update GetGCPPDOperatorControllerConfig to use a lowercase error message and remove the redundant klog.Error call; return the error directly so RunOperator remains the single logging point.
🧹 Nitpick comments (3)
pkg/driver/gcp-pd/gcp_pd.go (1)
74-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the selector closure.
cfg.StorageClassSelectorcan be a direct return instead of an if/return pair.♻️ Proposed simplification
cfg.StorageClassSelector = func(name string) bool { - if storageClassSet.Has(name) { - return true - } - return false + return storageClassSet.Has(name) }🤖 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/driver/gcp-pd/gcp_pd.go` around lines 74 - 85, In the cfg.StorageClassSelector closure, replace the redundant if/return branching with a direct return of storageClassSet.Has(name), preserving the existing selector behavior.pkg/driver/gcp-pd/storageclass_test.go (2)
119-129: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winTest takes ~10 seconds due to the context timeout.
Since
PollUntilContextTimeoutruns immediately (immediate=true) and the poll interval is 5s, a much shorter context (e.g. 1-2s) would still exercise the not-found error path without waiting out most of the 10s window.⚡ Proposed fix
configClient := fakeconfig.NewSimpleClientset() // Use a short timeout so the retry loop finishes quickly in tests. - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) defer cancel()🤖 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/driver/gcp-pd/storageclass_test.go` around lines 119 - 129, Shorten the timeout context in TestGetStorageClassFilesNoInfrastructure to approximately 1–2 seconds, keeping the existing immediate polling and expected-error assertion unchanged so the not-found path is exercised without the test taking nearly 10 seconds.
98-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeprecated
fakeconfig.NewSimpleClientset.Static analysis flags this as deprecated (
SA1019) in favor ofNewClientset, which may fail lint gating in CI.🧰 Tools
golangci-lint (2.12.2): `SA1019: fakeconfig.NewSimpleClientset is deprecated: NewClientset replaces this...`Also applies to: 120-120
🤖 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/driver/gcp-pd/storageclass_test.go` at line 98, Replace deprecated fakeconfig.NewSimpleClientset calls in the storage class tests with fakeconfig.NewClientset, including both occurrences, while preserving the existing test infrastructure arguments and behavior.Source: Linters/SAST tools
🤖 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 `@pkg/driver/gcp-pd/gcp_pd.go`:
- Around line 92-133: Update the error handling after
wait.PollUntilContextTimeout in getStorageClassFiles so it wraps lastErr only
when it is non-nil; otherwise use the poll’s err as the underlying error.
Preserve the existing failure message while ensuring the returned error never
wraps a nil value.
---
Duplicate comments:
In `@pkg/driver/gcp-pd/gcp_pd.go`:
- Around line 51-55: Update GetGCPPDOperatorControllerConfig to use a lowercase
error message and remove the redundant klog.Error call; return the error
directly so RunOperator remains the single logging point.
---
Nitpick comments:
In `@pkg/driver/gcp-pd/gcp_pd.go`:
- Around line 74-85: In the cfg.StorageClassSelector closure, replace the
redundant if/return branching with a direct return of storageClassSet.Has(name),
preserving the existing selector behavior.
In `@pkg/driver/gcp-pd/storageclass_test.go`:
- Around line 119-129: Shorten the timeout context in
TestGetStorageClassFilesNoInfrastructure to approximately 1–2 seconds, keeping
the existing immediate polling and expected-error assertion unchanged so the
not-found path is exercised without the test taking nearly 10 seconds.
- Line 98: Replace deprecated fakeconfig.NewSimpleClientset calls in the storage
class tests with fakeconfig.NewClientset, including both occurrences, while
preserving the existing test infrastructure arguments and behavior.
🪄 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: 18167f75-4491-460d-9f98-79d63f46f342
⛔ Files ignored due to path filters (33)
assets/overlays/gcp-pd/generated/standalone/cabundle_cm.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller_hostnetwork_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller_pdb.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/controller_sa.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/csidriver.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/hostnetwork_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/kube_rbac_proxy_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/kube_rbac_proxy_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/lease_leader_election_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/lease_leader_election_rolebinding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_attacher_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_provisioner_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_resizer_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/main_snapshotter_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/manifests.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/node.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/node_privileged_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/node_sa.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/privileged_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/prometheus_role.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/prometheus_rolebinding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/service.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/servicemonitor.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/storageclass.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/storageclass_hyperdisk_balanced.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/storageclass_reader_resizer_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/storageclass_ssd.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumeattributesclass_reader_provisioner_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumeattributesclass_reader_resizer_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumesnapshot_reader_provisioner_binding.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumesnapshotclass.yamlis excluded by!**/generated/**assets/overlays/gcp-pd/generated/standalone/volumesnapshotclass_images.yamlis excluded by!**/generated/**
📒 Files selected for processing (8)
Dockerfile.gcp-pdassets/overlays/gcp-pd/custom/old_controller_privileged_binding.yamlcmd/gcp-pd-csi-driver-operator/main.gopkg/driver/gcp-pd/gcp_pd.gopkg/driver/gcp-pd/storageclass_test.gopkg/generated-assets/generated_assets.gopkg/operator/config/config.gopkg/operator/starter.go
🚧 Files skipped from review as they are similar to previous changes (2)
- Dockerfile.gcp-pd
- cmd/gcp-pd-csi-driver-operator/main.go
| // getStorageClassFiles returns the list of StorageClass asset files to use, | ||
| // based on whether the cluster runs on GCP Dedicated. | ||
| // It retries for up to 1 minute to fetch the Infrastructure CR, because during | ||
| // early cluster installation the CR may not exist yet. | ||
| // On GCP Dedicated, only hyperdisk-balanced is supported. | ||
| // On regular GCP, standard-csi and ssd-csi are used. | ||
| func getStorageClassFiles(ctx context.Context, configClient configclient.Interface) ([]string, error) { | ||
| regularFiles := []string{ | ||
| "storageclass.yaml", | ||
| "storageclass_ssd.yaml", | ||
| } | ||
| gcpDedicatedFiles := []string{ | ||
| "storageclass_hyperdisk_balanced.yaml", | ||
| } | ||
|
|
||
| var region string | ||
| var lastErr error | ||
| err := wait.PollUntilContextTimeout(ctx, 5*time.Second, 1*time.Minute, true, func(ctx context.Context) (bool, error) { | ||
| infra, err := configClient.ConfigV1().Infrastructures().Get(ctx, globalInfrastructureName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| lastErr = err | ||
| klog.V(4).Infof("Failed to get Infrastructure CR, will retry: %v", err) | ||
| return false, nil | ||
| } | ||
| if infra.Status.PlatformStatus == nil || infra.Status.PlatformStatus.GCP == nil { | ||
| klog.V(4).Infof("Infrastructure CR has no GCP PlatformStatus, assuming regular GCP") | ||
| return true, nil | ||
| } | ||
| region = infra.Status.PlatformStatus.GCP.Region | ||
| return true, nil | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get Infrastructure CR: %w", lastErr) | ||
| } | ||
|
|
||
| if strings.HasPrefix(region, gcpDedicatedRegionPrefix) { | ||
| klog.Infof("GCP Dedicated detected (region %q), using hyperdisk-balanced StorageClass", region) | ||
| return gcpDedicatedFiles, nil | ||
| } | ||
| klog.Infof("Regular GCP detected (region %q), using standard StorageClasses", region) | ||
| return regularFiles, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and the polling helper usage context.
git ls-files pkg/driver/gcp-pd/gcp_pd.go
wc -l pkg/driver/gcp-pd/gcp_pd.go
cat -n pkg/driver/gcp-pd/gcp_pd.go | sed -n '1,220p'
# Find the imported wait package and any similar error-handling patterns.
rg -n "PollUntilContextTimeout|lastErr|fmt.Errorf\\(\"failed to get Infrastructure CR" -S pkg/driver/gcp-pd/gcp_pd.go pkg -g '!**/vendor/**'Repository: openshift/csi-operator
Length of output: 6768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Use a small read-only probe to learn PollUntilContextTimeout behavior from the
# vendored/module source if available, and from the public Go package docs if not.
python3 - <<'PY'
import os, glob
candidates = glob.glob('**/*wait*.go', recursive=True)
for p in candidates[:20]:
print(p)
PY
# Search for the helper implementation in downloaded modules if present.
rg -n "func PollUntilContextTimeout|PollUntilContextTimeout\\(" "$(go env GOPATH 2>/dev/null)/pkg/mod" -g '*.go' 2>/dev/null | head -n 40 || trueRepository: openshift/csi-operator
Length of output: 10789
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation of PollUntilContextTimeout in the vendored wait package.
sed -n '1,260p' vendor/k8s.io/apimachinery/pkg/util/wait/wait.go | cat -n
# Also inspect any helper paths around the specific function if needed.
rg -n "func PollUntilContextTimeout|ErrWaitTimeout|context deadline exceeded|context canceled|lastErr" vendor/k8s.io/apimachinery/pkg/util/wait/wait.go -n -A4 -B4Repository: openshift/csi-operator
Length of output: 10080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact PollUntilContextTimeout implementation with line numbers only.
awk '
/func PollUntilContextTimeout/ {flag=1}
flag {print NR ":" $0}
flag && /^\}/ && c++>=1 {exit}
' vendor/k8s.io/apimachinery/pkg/util/wait/wait.go
# Probe how fmt.Errorf behaves with %w and a nil error in a minimal Go snippet.
cat <<'EOF' >/tmp/probe_nil_wrap.go
package main
import (
"fmt"
)
func main() {
var err error
fmt.Printf("%q\n", fmt.Errorf("failed: %w", err).Error())
}
EOF
go run /tmp/probe_nil_wrap.goRepository: openshift/csi-operator
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any local callers that rely on this exact error message shape.
rg -n "failed to get Infrastructure CR" -S .Repository: openshift/csi-operator
Length of output: 404
Avoid wrapping lastErr unconditionally. If the poll exits before the callback sets lastErr, %w will render a malformed %!w(<nil>) message. Fall back to err when lastErr is nil.
🤖 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/driver/gcp-pd/gcp_pd.go` around lines 92 - 133, Update the error handling
after wait.PollUntilContextTimeout in getStorageClassFiles so it wraps lastErr
only when it is non-nil; otherwise use the poll’s err as the underlying error.
Preserve the existing failure message while ensuring the returned error never
wraps a nil value.
|
/retest-required |
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| configClient := fakeconfig.NewSimpleClientset(test.infra) | ||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
|
|
||
| files, err := getStorageClassFiles(ctx, configClient) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| if len(files) != len(test.expectedFiles) { | ||
| t.Fatalf("expected %d files, got %d: %v", len(test.expectedFiles), len(files), files) | ||
| } | ||
| for i, f := range files { | ||
| if f != test.expectedFiles[i] { | ||
| t.Errorf("file[%d]: expected %q, got %q", i, test.expectedFiles[i], f) | ||
| } | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Just a suggestion, could we possibly include assert library and use it for unit tests? I think we lack use of this library in our repos and for unit tests this is a great tool.
Then the code could look for example something like this (this is just an example, it can definitely be done better):
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
configClient := fakeconfig.NewSimpleClientset(test.infra)
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
files, err := getStorageClassFiles(ctx, configClient)
//using assert
assert.NoErrorf(t, err, "unexpected error: %v", err)
assert.Len(t,test.expectedFiles, len(files), "wrong number of files")
assert.Equal(t, test.expectedFiles, files, "files don't match expected")
})
}
|
/retest-required |
|
@mpatlasov I don't mind if we don't have the code to generate assets right now, I was just unsure why it was not there yet, we can merge this PR without the generation code. I left a unit test suggestion, but we do not need to switch to the library right now as getting this PR merged is a priority. I don't think the e2e tests are failing because of changes from this pr and required ones seem to be passing. |
|
@mpatlasov: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest-required |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-gcp-ovn-csi |
|
@mpatlasov: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e2809ef0-805d-11f1-8e7a-27de9981ab15-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-gcp-ovn |
|
@mpatlasov: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-gcp-ovn-serial |
|
@mpatlasov: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/bebacb70-805e-11f1-87c5-2a251edf5348-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-gcp-ovn-serial |
|
/retest all |
|
@mpatlasov: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/56f94b20-8085-11f1-9e6f-d576eba2878f-0 |
|
/retest |
5 participants
This PR implements the minimal set of go-files and yaml-assets to enable the command
to install and run GCP PD CSI driver:
/cc @openshift/storage