fix(cli): add --header flag to all CLI subcommands#1444
fix(cli): add --header flag to all CLI subcommands#1444capable-average wants to merge 3 commits intoraystack:mainfrom
Conversation
|
@capable-average is attempting to deploy a commit to the Raystack Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces inline Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
|
closes #1392 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/organization.go (1)
296-346:⚠️ Potential issue | 🟡 MinorMissing header flag in
admlistOrganizationCommand.This command still uses
connect.NewRequestdirectly (line 317) and lacks the--headerflag. Per the PR objective to "add --header flag to all CLI subcommands," this command should also be refactored to use thenewRequesthelper with header support.🔧 Proposed fix to add header support
func admlistOrganizationCommand(cliConfig *Config) *cli.Command { + var header string cmd := &cli.Command{ Use: "admlist", Short: "list admins of an organization", @@ ... @@ organizationID := args[0] - res, err := client.ListOrganizationAdmins(cmd.Context(), connect.NewRequest(&frontierv1beta1.ListOrganizationAdminsRequest{ + req, err := newRequest(&frontierv1beta1.ListOrganizationAdminsRequest{ Id: organizationID, - })) + }, header) + if err != nil { + return err + } + res, err := client.ListOrganizationAdmins(cmd.Context(), req) if err != nil { @@ ... @@ }, } + cmd.Flags().StringVarP(&header, "header", "H", "", "Header <key>:<value>") + return cmd }Additionally, once
admlistOrganizationCommandis refactored, theconnectimport on line 7 can be removed since no other function in this file will use it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55d41d00-ccdb-45bc-b0d2-95fc2a6f9e9a
📒 Files selected for processing (8)
cmd/group.gocmd/namespace.gocmd/organization.gocmd/permission.gocmd/policy.gocmd/project.gocmd/role.gocmd/user.go
|
@capable-average thanks for contributing. Could you have a look at one command that got missed,
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 705
func admlistOrganizationCommand(cliConfig *Config) *cli.Command {
var header string
cmd := &cli.Command{
Use: "admlist",
Short: "list admins of an organization",
// ... existing fields ...
RunE: func(cmd *cobra.Command, args []string) error {
// ... existing client setup ...
organizationID := args[0]
req, err := newRequest(&frontierv1beta1.ListOrganizationAdminsRequest{
Id: organizationID,
}, header)
if err != nil {
return err
}
res, err := client.ListOrganizationAdmins(cmd.Context(), req)
if err != nil {
// ... existing error handling ...
}
// ... rest of existing code ...
},
}
cmd.Flags().StringVarP(&header, "header", "H", "", "Header <key>:<value>")
return cmd
}Additionally, once this is updated, the |
|
sure, can you also please confirm that '-H' being a required flag is intended? |
capable-average
force-pushed
the
fix/add-header-flag-in-cli-subcommands
branch
from
bdf9d10 to
b80aee5
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/user.go (1)
98-101:⚠️ Potential issue | 🟡 MinorInconsistent:
--headeris required only increateUserCommand.The
--headerflag is marked as required here (line 101) but is optional ineditUserCommand,viewUserCommand, andlistUserCommand. This inconsistency was also raised in the PR comments. If the header is meant to be optional across all commands (matching namespace.go), remove theMarkFlagRequiredcall.Proposed fix to make header optional
cmd.Flags().StringVarP(&filePath, "file", "f", "", "Path to the user body file") cmd.MarkFlagRequired("file") cmd.Flags().StringVarP(&header, "header", "H", "", "Header <key>:<value>") - cmd.MarkFlagRequired("header")cmd/role.go (1)
94-97:⚠️ Potential issue | 🟡 MinorInconsistent:
--headeris required only increateRoleCommand.Same issue as
createUserCommand- the header flag is marked as required here but is optional ineditRoleCommand,viewRoleCommand, andlistRoleCommand. This creates an inconsistent user experience where only create operations mandate headers.Proposed fix to make header optional
cmd.Flags().StringVarP(&filePath, "file", "f", "", "Path to the role body file") cmd.MarkFlagRequired("file") cmd.Flags().StringVarP(&header, "header", "H", "", "Header <key>:<value>") - cmd.MarkFlagRequired("header")
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55a22f37-b3c5-4401-bb6c-cc37c85e4dc5
📒 Files selected for processing (8)
cmd/group.gocmd/namespace.gocmd/organization.gocmd/permission.gocmd/policy.gocmd/project.gocmd/role.gocmd/user.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/permission.go
- cmd/project.go
- cmd/policy.go
- cmd/group.go
- cmd/organization.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/organization.go (1)
96-97:⚠️ Potential issue | 🟠 MajorInconsistent:
--headeris required only forcreate, optional elsewhere.The
--headerflag is marked as required here but is optional inedit,view,list, andadmlistcommands. ThenewRequesthelper already handles empty headers gracefully by simply not setting any header. This was also raised in the PR comments.Consider removing the required constraint for consistency:
Suggested fix
cmd.Flags().StringVarP(&header, "header", "H", "", "Header <key>:<value>") - cmd.MarkFlagRequired("header")
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9851f46e-67ff-4269-90c3-0995b8e8f4ee
📒 Files selected for processing (1)
cmd/organization.go
|
@capable-average Yes, the |
capable-average
force-pushed
the
fix/add-header-flag-in-cli-subcommands
branch
from
ea928d4 to
e30d271
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cmd/seed.go (1)
56-65: Conflicting flag logic:--headeris marked required but fallback logic for missing header remains.Line 92 marks
--headeras required withcmd.MarkFlagRequired("header"), yet lines 56-65 contain a fallback path that loads the header from config when it's empty. With the required flag enforcement, this fallback code and the--configflag (line 93) become unreachable in normal CLI flow. Remove the fallback logic and config flag if--headershould always be required, or remove theMarkFlagRequired()call if the fallback behavior is intentional.cmd/organization.go (1)
231-232: Consider whether--headershould be required for read-only commands.Making
--headerrequired forviewandlistcommands may degrade CLI usability since users must always provide a header even for simple read operations. If the header is only needed occasionally (e.g., for authentication), consider making it optional for these commands.This applies similarly to
listOrganizationCommand(lines 292-293) andadmlistOrganizationCommand(lines 352-353).cmd/project.go (1)
231-232: Same UX consideration as organization commands.As noted in
cmd/organization.go, making--headerrequired for read-onlyviewandlistcommands may impact usability. This applies to lines 231-232 and 295-296.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23f9b34a-b5eb-4a93-a454-6b217565b193
📒 Files selected for processing (15)
cmd/group.gocmd/group_test.gocmd/namespace.gocmd/organization.gocmd/organization_test.gocmd/permission.gocmd/permission_test.gocmd/policy.gocmd/policy_test.gocmd/project.gocmd/project_test.gocmd/role.gocmd/role_test.gocmd/seed.gocmd/user.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/namespace.go
- cmd/role.go
- cmd/user.go
- cmd/permission.go
- cmd/policy.go
3 participants
Summary
Test Plan