fix(preheat): handle nil payload during policy update#1022
fix(preheat): handle nil payload during policy update#1022shellyco-code wants to merge 3 commits into
Conversation
Signed-off-by: shellyco-code <shellychahar57@gmail.com>
NucleoFusion
left a comment
NucleoFusion
left a comment
There was a problem hiding this comment.
Testing for cmds is limited to testing for failures. By that I mean that we induce errors by providing malformed input/flags to check whether the command appropriately fails.
By that I mean that providing invalid flags, too large values etc.
The PR resolves the issue, but tests are not up to the general CLI standard, all else looks great!
| var ( | ||
| getPreheatPolicyFunc = api.GetPreheatPolicy | ||
| ) | ||
|
|
There was a problem hiding this comment.
we generally discourage this method of testing since it makes it kinda complicated to understand.
Our method for testing is to just test the command failures, which is wrong flags, malformed input etc
The full command testing is TBD in the e2e tests which are in-dev by a support feature that @bupd i think is making.
There was a problem hiding this comment.
We can just remove this now, and the relevant things in tests, cause we dont use it
| var buf bytes.Buffer | ||
| cmd.SetOut(&buf) | ||
| cmd.SetErr(&buf) | ||
| cmd.SetArgs([]string{"my-project", "my-policy"}) |
There was a problem hiding this comment.
There is a test helper function, in /pkg/testutil/testutil.go
With function signature,
func TestCmd(t *testing.T, cmdFunc func() *cobra.Command, flags ...string) error {
This takes the cmd & the flags, and just outputs the error (if any)
so you can just use this
There was a problem hiding this comment.
Thanks for the feedback! I've updated the tests to use the existing pkg/testutil.TestCmd helper and aligned them with the Harbor CLI testing conventions. I also verified that the test suite passes after the changes. Please let me know if there's anything else I should adjust.
| var ( | ||
| getPreheatPolicyFunc = api.GetPreheatPolicy | ||
| ) | ||
|
|
There was a problem hiding this comment.
We can just remove this now, and the relevant things in tests, cause we dont use it
| err := testutil.TestCmd(t, UpdatePolicyCommand, "my-project", "my-policy") | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "payload is empty") | ||
| } | ||
|
|
||
| func TestUpdatePolicyCommand_NilPayload(t *testing.T) { | ||
| originalGetPreheatPolicy := getPreheatPolicyFunc | ||
| t.Cleanup(func() { | ||
| getPreheatPolicyFunc = originalGetPreheatPolicy | ||
| }) | ||
|
|
||
| getPreheatPolicyFunc = func(projectName, policyName string) (*preheat.GetPolicyOK, error) { | ||
| return &preheat.GetPolicyOK{Payload: nil}, nil | ||
| } | ||
|
|
||
| err := testutil.TestCmd(t, UpdatePolicyCommand, "my-project", "my-policy") | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "payload is empty") |
There was a problem hiding this comment.
These tests will fail now
cause we are not using the pr.
Have a look at cmd/harbor/root/project/list_test.go
We test only the cases for flags and stuff like that, we dont test for cases that propagate back errors from a more deeper level package
There was a problem hiding this comment.
Thanks for the detailed feedback! That makes sense. I'll remove the getPreheatPolicyFunc indirection and update the tests to follow the Harbor CLI testing conventions, using list_test.go as a reference. I'll push an updated revision shortly. Thanks!
|
DCO is failing btw |
Signed-off-by: shellyco-code <shellychahar57@gmail.com>
shellyco-code
force-pushed
the
fix/preheat-policy-nil-payload
branch
from
874bc48 to
7610a7e
Compare
Signed-off-by: shellyco-code <shellychahar57@gmail.com>
NucleoFusion
left a comment
NucleoFusion
left a comment
There was a problem hiding this comment.
lgtm! thanks for the contribution!
NucleoFusion
left a comment
NucleoFusion
left a comment
There was a problem hiding this comment.
lgtm!
Thanks for the contribution!
qcserestipy
added
the
status/has-approved-pr
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1022 +/- ##
=========================================
- Coverage 10.99% 9.45% -1.55%
=========================================
Files 173 321 +148
Lines 8671 16084 +7413
=========================================
+ Hits 953 1520 +567
- Misses 7612 14430 +6818
- Partials 106 134 +28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
Description
This pull request fixes a runtime nil pointer dereference panic in the
harbor project preheat policy updatecommand when the fetched preheat policy or its payload from the API is nil/empty.Type of Change
Changes
cmd/harbor/root/project/preheat/policy/update.goto validate ifexistingPolicyandexistingPolicy.Payloadare nil, returning a clean error instead of panicking.api.GetPreheatPolicycall to use a package-level mockable variablegetPreheatPolicyFuncto enable clean unit testing.cmd/harbor/root/project/preheat/policy/update_test.goto add unit tests mocking nil policy and nil payload API return values.Testing
gofmt -s -w.go build).go test ./cmd/harbor/root/project/preheat/policy/...), which passed successfully.go test ./...), which passed successfully without regressions.