fix(cli): add --header flag to all CLI subcommands#1444
fix(cli): add --header flag to all CLI subcommands#1444capable-average wants to merge 2 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🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 |
|
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
2 participants
Summary
Test Plan