-
Notifications
You must be signed in to change notification settings - Fork 70
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
add org list roles cmd #1554
add org list roles cmd #1554
Conversation
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1554 +/- ##
==========================================
+ Coverage 86.14% 86.21% +0.07%
==========================================
Files 112 113 +1
Lines 14945 15026 +81
==========================================
+ Hits 12874 12955 +81
Misses 1252 1252
Partials 819 819 ☔ View full report in Codecov by Sentry. |
cloud/role/role.go
Outdated
if err != nil { | ||
return nil, nil, err | ||
} | ||
defaultRoles = *resp.JSON200.DefaultRoles |
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.
nit can we do it so we only set this once and after the first ListRolesWithResponse
call, we turn includeDefaultRoles to false
since we already have em
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.
actually should we add a flag as to whether to include it or not,
seems kinda annoying to have it always show up if you only care about custom roles
im thinking --include-default-roles
should even be default false 🤔
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 call. done.
cmd/cloud/organization.go
Outdated
func newOrganizationRoleRootCmd(out io.Writer) *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "role", | ||
Aliases: []string{"te", "roles"}, |
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.
what's te
😅
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.
team 😂. I just changed it to ro thx
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.
thanks for the changes
after this is merged, can we get some qa testing on this @shri-astro @sreenusuuda
thank you :D
Description
🎟 Issue(s)
Related #XXX
🧪 Functional Testing
Tested the functionality of the cmd locally
📸 Screenshots
📋 Checklist
make test
before taking out of draftmake lint
before taking out of draft