Skip to content

fix: handle nil CopyByChunk in replication policy view#596

Merged
bupd merged 2 commits into
goharbor:mainfrom
yimtun:fix/copy-by-chunk-nil-check
Mar 8, 2026
Merged

fix: handle nil CopyByChunk in replication policy view#596
bupd merged 2 commits into
goharbor:mainfrom
yimtun:fix/copy-by-chunk-nil-check

Conversation

@yimtun

@yimtun yimtun commented Jan 14, 2026

Copy link
Copy Markdown
Contributor

What this PR does

  • Add a helper function to safely format CopyByChunk
  • Avoid potential nil pointer dereference when CopyByChunk is not set

Why

ReplicationPolicy.CopyByChunk is optional and may be nil for some replication policies.
Calling strconv.FormatBool(*CopyByChunk) in such cases causes a panic.

This issue was observed when using Harbor server version 2.5.5.

This issue can be reproduced by running:

harbor-cli replication policies view 1 -v

Which results in:

INFO[2026-01-14T04:01:54-05:00] System keyring not available, using file-based keyring 
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xa1ed09]

goroutine 1 [running]:
github.com/goharbor/harbor-cli/pkg/views/replication/policies/view.ViewPolicy(0xc0002b2d00)
        /src/pkg/views/replication/policies/view/view.go:71 +0x669
github.com/goharbor/harbor-cli/cmd/harbor/root/replication.ReplicationPoliciesCommand.ViewCommand.func1(0xc000255300?, {0xc0001cd060?, 0x4?, 0xc4b36f?})
        /src/cmd/harbor/root/replication/policies/view.go:59 +0x13d
github.com/spf13/cobra.(*Command).execute(0xc00024f808, {0xc0001cd040, 0x2, 0x2})
        /go/pkg/mod/github.com/spf13/cobra@v1.10.1/command.go:1015 +0xb02
github.com/spf13/cobra.(*Command).ExecuteC(0xc000220008)
        /go/pkg/mod/github.com/spf13/cobra@v1.10.1/command.go:1148 +0x465
github.com/spf13/cobra.(*Command).Execute(0xc000002380?)
        /go/pkg/mod/github.com/spf13/cobra@v1.10.1/command.go:1071 +0x13
main.main()
        /src/cmd/harbor/main.go:23 +0x18

How

  • Extract formatting logic into formatCopyByChunk
  • Return "N/A" when the value is nil

Notes

  • No behavior change for non-nil values
  • Improves code readability and robustness

@yimtun yimtun force-pushed the fix/copy-by-chunk-nil-check branch from ae52da7 to b6f6fa4 Compare January 14, 2026 12:03
@yimtun

yimtun commented Jan 16, 2026

Copy link
Copy Markdown
Contributor Author

PTAL @bupd @Vad1mo thanks!

bupd
bupd approved these changes Jan 20, 2026
View reviewed changes

@bupd bupd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

nice catch. @yimtun Thanks

@bupd bupd self-assigned this Jan 20, 2026
@bupd bupd added bug Something isn't working go Pull requests that update go code labels Jan 20, 2026
@codecov

codecov Bot commented Jan 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 7.47%. Comparing base (60ad0bd) to head (55ae809).
⚠️ Report is 110 commits behind head on main.

Files with missing lines Patch % Lines
pkg/views/replication/policies/view/view.go 80.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##             main    #596      +/-   ##
=========================================
- Coverage   10.99%   7.47%   -3.53%     
=========================================
  Files         173     261      +88     
  Lines        8671   13012    +4341     
=========================================
+ Hits          953     972      +19     
- Misses       7612   11931    +4319     
- Partials      106     109       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yimtun

yimtun commented Jan 22, 2026

Copy link
Copy Markdown
Contributor Author

@bupd Hi I’ve added unit tests covering both nil and non-nil CopyByChunk cases in this PR.

@yimtun yimtun requested a review from bupd January 22, 2026 02:34
bupd
bupd requested changes Feb 3, 2026
View reviewed changes

@bupd bupd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please fix lint - add license header to the new view_test.go file

Comment thread pkg/views/replication/policies/view/view_test.go
@yimtun yimtun force-pushed the fix/copy-by-chunk-nil-check branch from 693b139 to b7c3a6e Compare March 6, 2026 13:16
@yimtun
fix: handle nil CopyByChunk in replication policy view
8617a1f 
Signed-off-by: yimtun <yim.tune@gmail.com>
@yimtun yimtun force-pushed the fix/copy-by-chunk-nil-check branch from b7c3a6e to 8617a1f Compare March 6, 2026 13:27
@yimtun yimtun requested a review from bupd March 6, 2026 13:31
@bupd
fix: handle nil Speed pointer in replication policy view
55ae809 
Signed-off-by: bupd <bupdprasanth@gmail.com>
bupd
bupd approved these changes Mar 8, 2026
View reviewed changes

@bupd bupd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@bupd bupd merged commit 999391c into goharbor:main Mar 8, 2026
7 of 8 checks passed
@BrewTestBot BrewTestBot mentioned this pull request Apr 21, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bupd bupd bupd approved these changes

Assignees

@bupd bupd

Labels

bug Something isn't working go Pull requests that update go code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yimtun @bupd