fix: project view --id flag not working#639
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #639 +/- ##
=========================================
- Coverage 10.99% 7.24% -3.75%
=========================================
Files 173 260 +87
Lines 8671 12898 +4227
=========================================
- Hits 953 935 -18
- Misses 7612 11855 +4243
- Partials 106 108 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@bupd @NucleoFusion please review. |
qcserestipy
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I added a few comments. It would make the code simpler by removing the CheckProject function and replace the logic where it is used with GetProject.
bad54ba to
4428927
Compare
|
@qcserestipy |
|
@qcserestipy any update on this? |
|
LGTM, can you add a screenshot or screen record that documents the outcome? |
Signed-off-by: Sypher845 <suyashpatil845@gmail.com>
Signed-off-by: Sypher845 <suyashpatil845@gmail.com>
|
@qcserestipy done, added the GIF and also updated the PR description |
|
Nice, thank you! LGTM |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #638 where the --id flag for harbor project view was defined but not functional. The fix enhances error parsing to support additional error formats and refactors project existence checks to properly use the isID parameter.
Changes:
- Enhanced
ParseHarborErrorCodeto parse errors in(status CODE)format in addition to the existing[METHOD /path][CODE]format - Removed the
CheckProjectfunction and replaced all usages withGetProject, which supports theisIDparameter - Fixed
project viewcommand to properly pass theisIDflag toGetProject, enabling project lookup by ID
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/error.go | Added regex-based parsing for (status CODE) error format |
| pkg/api/project_handler.go | Removed unused CheckProject function |
| cmd/harbor/root/project/view.go | Fixed to properly use isID flag when fetching projects |
| cmd/harbor/root/project/member/delete.go | Replaced CheckProject with GetProject to support isID flag |
| cmd/harbor/root/project/member/create.go | Replaced CheckProject with GetProject to support isID flag |
| cmd/harbor/root/project/logs.go | Replaced CheckProject with GetProject for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Try format: (status CODE) - e.g., (status 404) | ||
| re := regexp.MustCompile(`\(status\s+(\d{3})\)`) |
There was a problem hiding this comment.
The regex is compiled on every function call, which is inefficient. Consider compiling the regex once as a package-level variable to improve performance, especially if this function is called frequently.
There was a problem hiding this comment.
The function is not called frequently. Also many other files are compiling the regex inside the function.
There was a problem hiding this comment.
SHould i implement this suggestion? @bupd @qcserestipy
Signed-off-by: Sypher845 <suyashpatil845@gmail.com>
4428927 to
1f6b16f
Compare
Fixes #638
In
pkg/utils/error.go, the functionParseHarborErrorCodecan now parse error code of type(status CODE), before it was only able to parse error of type[METHOD /path][CODE].Removed the
CheckProjectfunction inpkg/api/project_handler.goand replaced the logic where it is used withGetProjectBefore the --id flag for project view was not working , now it correctly passes
isIDtoGetProjectand works as expected.