-
Notifications
You must be signed in to change notification settings - Fork 133
refactor(robot): dynamically fetch and sort permissions with improved error handling #690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… error handling - Fetch available permissions and actions from API instead of using hardcoded values - Sort resources and actions deterministically for reliable UI presentation - Introduce PermissionSelectResult struct for proper error propagation in channel - Support both 'project' and 'system' permission kinds - Remove os.Exit() calls in favor of channel-based error handling Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
=========================================
- Coverage 10.99% 7.16% -3.83%
=========================================
Files 173 260 +87
Lines 8671 12864 +4193
=========================================
- Hits 953 922 -31
- Misses 7612 11834 +4222
- Partials 106 108 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
NucleoFusion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good!
We could also handle the api.GetPermissions() error here in my opinion
pkg/prompt/prompt.go
Outdated
| response, _ := api.GetPermissions() | ||
| robotView.ListPermissions(response.Payload, kind, permissions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we integrate the api.GetPermissions error to be returned here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good hint! I added error handling for this one. In a next PR I will deal with all the unhandled errors in Robot account stuff in prompt.go
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
NucleoFusion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
bupd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Overview
This PR does not change the functionality, but refactors the
NewRobotPermissionsGridin the robot permission selection. Before it was relying on hard coded actions and permissions for the table creation. This information is now retrieved directly from the harbor api as the permission spec might change in the future and hard coded parts break. Additionally, the go routine passes a customn struct that allows for more cleaner error handling than justos.Exiton failureChanges