Skip to content

Fix Panics from BeforeServe during Boot#462

Merged
falfaroc merged 4 commits into
mainfrom
usr/falfaroc/remove-panics
Apr 10, 2025
Merged

Fix Panics from BeforeServe during Boot#462
falfaroc merged 4 commits into
mainfrom
usr/falfaroc/remove-panics

Conversation

@falfaroc

@falfaroc falfaroc commented Apr 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Return the error instead of performing a panic in the BeforeServe. This was causing misleading errors and CrashLoopBackOff for the controller Pod when the X_CSI_NODE_NAME is not present.

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
https://github.com/dell/csm/issues/1742

Checklist:

  • I have performed a self-review of my own code to ensure there are no formatting, vetting, linting, or security issues
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate
  • I have maintained at least 90% code coverage
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • Backward compatibility is not broken

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

  • Ensure that unit tests pass.
  • Manually install PowerStore with Operator and ensure that the missing parameter no longer panics.
  • Ensure that Operator e2e tests pass accordingly.

@falfaroc falfaroc force-pushed the usr/falfaroc/remove-panics branch from 3fc742d to d5a4e7f Compare April 10, 2025 20:02
@falfaroc falfaroc marked this pull request as ready for review April 10, 2025 20:09
xuluna
xuluna reviewed Apr 10, 2025
View reviewed changes
Comment thread pkg/service/node.go Outdated
@github-actions

github-actions Bot commented Apr 10, 2025

Copy link
Copy Markdown
Contributor

Merging this branch will not change overall coverage

Impacted Packages Coverage Δ 🤖
github.com/dell/csi-powerstore/pkg/service 0.00% (ø)
Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/dell/csi-powerstore/pkg/service/node.go 0.00% (ø) 0 0 0
github.com/dell/csi-powerstore/pkg/service/service.go 0.00% (ø) 0 0 0

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

  • github.com/dell/csi-powerstore/pkg/service/service_test.go

@falfaroc falfaroc merged commit 5125615 into main Apr 10, 2025
@falfaroc falfaroc deleted the usr/falfaroc/remove-panics branch April 10, 2025 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abhi16394 abhi16394 Awaiting requested review from abhi16394

@adarsh-dell adarsh-dell Awaiting requested review from adarsh-dell

@AkshaySainiDell AkshaySainiDell Awaiting requested review from AkshaySainiDell

@donatwork donatwork Awaiting requested review from donatwork

2 more reviewers

@xuluna xuluna xuluna approved these changes

@santhoshatdell santhoshatdell santhoshatdell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@falfaroc @xuluna @santhoshatdell