fix(api): propagate errors from GetLabel instead of returning nil#1004
Open
AdeshDeshmukh wants to merge 1 commit into
Open
fix(api): propagate errors from GetLabel instead of returning nil#1004AdeshDeshmukh wants to merge 1 commit into
AdeshDeshmukh wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the label retrieval API to return errors instead of silently returning nil, and propagates that change to CLI commands that depend on label lookup.
Changes:
- Change
api.GetLabelto return(*models.Label, error)and propagate client/context errors. - Update
labels updatecommand to handleGetLabelerrors instead of checking fornil. - Update
artifact label addcommand to handleGetLabelerrors and format Harbor error messages.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/api/label_handler.go | Adjusts GetLabel to return an error on failure rather than nil. |
| cmd/harbor/root/labels/update.go | Updates label update flow to handle GetLabel error return value. |
| cmd/harbor/root/artifact/label/add.go | Updates artifact label add flow to handle GetLabel errors and parse Harbor error messages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+71
to
74
| existingLabel, err := api.GetLabel(labelId) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get label: %v", err) | ||
| } |
| return fmt.Errorf("label is not found") | ||
| existingLabel, err := api.GetLabel(labelId) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get label: %v", err) |
Contributor
There was a problem hiding this comment.
irrelevant, this in fact increases error distinction.
AdeshDeshmukh
force-pushed
the
fix/getlabel-return-error
branch
from
f8490ed to
d713250
Compare
NucleoFusion
suggested changes
Jun 20, 2026
Comment on lines
116
to
121
| if err != nil { | ||
| return nil | ||
| return nil, err | ||
| } | ||
|
|
||
| return response.GetPayload() | ||
| return response.GetPayload(), nil | ||
| } |
Contributor
There was a problem hiding this comment.
Consider adding a distinction for label being returned nil / 0
As label not found.
GetLabel silently returned nil on every error, discarding all error context. This caused callers to receive nil with no way to distinguish a network failure from a missing label. In the artifact label add command this led to a nil pointer dereference when accessing label.Name. Changes: - pkg/api/label_handler.go: GetLabel now returns (*models.Label, error). Both error paths propagate the actual error instead of nil. - cmd/harbor/root/labels/update.go: Proper error handling with ParseHarborErrorMsg for consistent API error formatting. - cmd/harbor/root/artifact/label/add.go: Added error check after GetLabel so label.Name is only accessed with a valid label. Signed-off-by: Adesh Deshmukh <adeshkd123@gmail.com>
AdeshDeshmukh
force-pushed
the
fix/getlabel-return-error
branch
from
d713250 to
4ef7586
Compare
Author
|
@NucleoFusion Done. Thank you for pointing out that edge case! The nil / 0 distinction is now fully handled. PTAL when you have a chance. |
NucleoFusion
approved these changes
Jun 21, 2026
NucleoFusion
left a comment
NucleoFusion
left a comment
Contributor
There was a problem hiding this comment.
lgtm!
Thanks for the contribution!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1003
GetLabel was the only API handler that returned a bare pointer with no error.
Every failure got replaced with nil, leaving callers blind to what happened.
In the artifact label add path this meant a nil pointer dereference when
the code later accessed label.Name.
The fix is straightforward — change the signature to return an error like
every other function in the API layer does, and update both callers to
handle it properly.
Changes:
Both error paths return the actual error instead of nil.
error check so users see the real error instead of "label is not found".
GetLabel call so label.Name is only accessed when we have a valid label.
All existing tests pass. No new dependencies introduced.