search

IBM / raksh

Seamlessly use VM based TEEs with Kubernetes for data-in use protection
Apache License 2.0
36 stars 9 forks source link

Remove redundant return of the error #20

Closed ssinha20 closed 4 years ago

ssinha20 commented 4 years ago

existing issues in crypto_test.go solved.

harche commented 4 years ago

@ssinha20 thanks for the PR.

Can you squash the commits?

harche commented 4 years ago

@bpradipt @mkumatag not sure why CI is failing.

mkumatag commented 4 years ago

@bpradipt @mkumatag not sure why CI is failing.

This is because of the go fmt issue:

$ make verify
./hack/verify-all.sh
./hack/..
Verify go-fmt!
diff -u ./pkg/crypto/crypto_test.go.orig ./pkg/crypto/crypto_test.go
--- ./pkg/crypto/crypto_test.go.orig    2020-06-04 11:08:17.396668930 +0000
+++ ./pkg/crypto/crypto_test.go 2020-06-04 11:08:17.396668930 +0000
@@ -108,4 +108,3 @@

    return string(plaintextBytes)
 }
-
Run ./hack/update-gofmt.sh
Makefile:113: recipe for target 'verify' failed
make: *** [verify] Error 1
The command "make verify" exited with 2.
mkumatag commented 4 years ago

And if I click on the overall changes, I don't see any of the unit testcases anymore, not sure whats the problem!

bpradipt commented 4 years ago

I think it would be better to close this PR and re-submit a new PR. I see the new test case TestDecryptConfigMapWrongkey deleted in the updated commit :-( or am I missing something ?

@harche @mkumatag ??

harche commented 4 years ago

@ssinha20 I was wondering for 3 lines of change which removes a redundant return of the error why do we need to have 8 commits? maybe we should squash those commits?

harche commented 4 years ago

I think it would be better to close this PR and re-submit a new PR. I see the new test case TestDecryptConfigMapWrongkey deleted in the updated commit :-( or am I missing something ?

I don't see TestDecryptConfigMapWrongkey getting deleted. In the changed files I see only one return of error being removed correctly.

ssinha20 commented 4 years ago

@harche : All commits are squashed into single commit now. can you please review and let me know your comments for the same.

ssinha20 commented 4 years ago

Regarding TestDecryptConfigMapWrongkey I have following work plan;

  1. handle in a separate PR.
  2. add TestDecryptConfigMapWrongkey in crypto_test.go. without changes in crypto.go; the TC PASSED with wrong key. Please suggest if this approach looks fine, then I will handle this in the new PR.
bpradipt commented 4 years ago

Regarding TestDecryptConfigMapWrongkey I have following work plan;

  1. handle in a separate PR.
  2. add TestDecryptConfigMapWrongkey in crypto_test.go. without changes in crypto.go; the TC PASSED with wrong key. Please suggest if this approach looks fine, then I will handle this in the new PR.

Please open new PR for specific changes.

ssinha20 commented 4 years ago

Sure Thanks.