Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add proper error handling and remove unnecessary variable definition #28

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

hlts2
Copy link
Member

@hlts2 hlts2 commented Nov 11, 2024

WHAT

I fixed an small issue where errors were not being handled correctly in the test code.
Additionally, I removed an unnecessary variable definition.

WHY

There were parts of the test code where errors were being handled, but no errors were assigned.
I believe this needs to be fixed.

  • e.g.) ./e2e/suite_test.go:104-113
    • we need to assign the result of the e2eTest.tenantClient.Update method to err variable

err = e2eTest.tenantClient.Get(context.TODO(), client.ObjectKey{Namespace: "kube-system", Name: "civo-csi-controller"}, controller)
if err != nil {
	log.Panic(err)
}

controller.Spec.Template.Spec.Containers[3].Image = Image

e2eTest.tenantClient.Update(context.TODO(), controller) <- we need to assign result to err variable
if err != nil {
	log.Panic(err)
}

I also found sections where the code worked correctly without the variable definition, so I removed it.

FYI

  • The following is the result of all unit test on Local environment
❯ go clean -testcache && go test ./...
ok  	github.com/civo/civo-csi	70.103s
ok  	github.com/civo/civo-csi/e2e	0.007s
ok  	github.com/civo/civo-csi/pkg/driver	10.013s

@hlts2 hlts2 self-assigned this Nov 11, 2024
@hlts2
Copy link
Member Author

hlts2 commented Nov 11, 2024

@Praveen005
Thank you for reviewing it!
I will merge this PR 🚀

@hlts2 hlts2 merged commit d37fbd7 into master Nov 11, 2024
4 checks passed
@hlts2 hlts2 deleted the fix/refactor-code branch November 18, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants