Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughA new CLI flag Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR adds a CLI-level mechanism for supplying user-defined HTTP headers that are automatically included on all OpenFGA API requests, addressing #669 for deployments that require non-standard auth headers.
Changes:
- Introduces a new
--custom-headerspersistent flag and wires it intoClientConfig. - Parses provided header strings into a
map[string]stringand passes them via the SDKDefaultHeadersconfiguration. - Extends the serialized client config model to include
custom_headers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
internal/fga/fga.go |
Adds CustomHeaders to config and applies parsed headers through the SDK client configuration. |
internal/cmdutils/get-client-config.go |
Reads the new custom-headers flag into fga.ClientConfig. |
cmd/root.go |
Registers the new persistent --custom-headers flag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/root.go
Outdated
| rootCmd.PersistentFlags().String("client-id", "", "Client ID. Sent to the Token Issuer during the Client Credentials flow") //nolint:lll | ||
| rootCmd.PersistentFlags().String("client-secret", "", "Client Secret. Sent to the Token Issuer during the Client Credentials flow") //nolint:lll | ||
| rootCmd.PersistentFlags().StringArray("api-scopes", []string{}, "API Scopes (repeat option for multiple values). Used in the Client Credentials flow") //nolint:lll | ||
| rootCmd.PersistentFlags().StringArray("custom-headers", []string{}, "Custom HTTP Headers (repeat option for multiple values)") //nolint:lll |
There was a problem hiding this comment.
The flag help text doesn’t describe the expected header format (e.g. Header: value). As implemented, values are parsed by splitting on the first :; without documenting this, users can easily pass a value that is silently ignored. Consider updating the description to explicitly state the required format and note that the flag can be repeated.
| rootCmd.PersistentFlags().StringArray("custom-headers", []string{}, "Custom HTTP Headers (repeat option for multiple values)") //nolint:lll | |
| rootCmd.PersistentFlags().StringArray("custom-headers", []string{}, "Custom HTTP headers in 'Header: value' format (repeat option for multiple values)") //nolint:lll |
| for _, header := range c.CustomHeaders { | ||
| parts := strings.SplitN(header, ":", 2) | ||
| if len(parts) == 2 { | ||
| head := strings.TrimSpace(parts[0]) | ||
| value := strings.TrimSpace(parts[1]) | ||
| headers[head] = value | ||
| } |
There was a problem hiding this comment.
Custom header parsing silently skips entries that don’t contain a :. This makes misconfiguration hard to detect (the CLI will run but the header won’t be sent). Consider validating CustomHeaders and returning an error (e.g. from GetFgaClient) when any entry is malformed.
internal/fga/fga.go
Outdated
| if len(parts) == 2 { | ||
| head := strings.TrimSpace(parts[0]) | ||
| value := strings.TrimSpace(parts[1]) | ||
| headers[head] = value |
There was a problem hiding this comment.
getCustomHeaders will accept an empty header name (e.g. input ": value"), resulting in a map entry with key "". It’s better to reject empty/whitespace-only header names during parsing/validation to avoid sending invalid HTTP headers.
| headers[head] = value | |
| if head != "" { | |
| headers[head] = value | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/root.go (1)
71-71: Clarify--custom-headersvalue format in help text.Consider documenting the expected format (for example,
Header-Name:Header Value) directly in the flag description to reduce invalid input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` at line 71, Update the flag description for rootCmd.PersistentFlags().StringArray("custom-headers", ...) to clarify the expected input format and reduce invalid values by documenting an example like "Header-Name: Header Value" and specifying that the colon separates name and value and the flag can be repeated for multiple headers; modify only the description string passed to StringArray for the "custom-headers" flag to include this format guidance and an example.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/fga/fga.go`:
- Around line 104-113: The getCustomHeaders method currently adds headers even
when the parsed header name is empty (e.g., ":value"); update
ClientConfig.getCustomHeaders to ignore entries with empty header names by
trimming parts[0] and checking head != "" before assigning headers[head] =
value, keeping the rest of the parsing logic the same so only valid non-empty
header keys are included.
---
Nitpick comments:
In `@cmd/root.go`:
- Line 71: Update the flag description for
rootCmd.PersistentFlags().StringArray("custom-headers", ...) to clarify the
expected input format and reduce invalid values by documenting an example like
"Header-Name: Header Value" and specifying that the colon separates name and
value and the flag can be repeated for multiple headers; modify only the
description string passed to StringArray for the "custom-headers" flag to
include this format guidance and an example.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 605667f4-98bb-4e92-9330-aac2d46d26b7
📒 Files selected for processing (3)
cmd/root.gointernal/cmdutils/get-client-config.gointernal/fga/fga.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (c ClientConfig) getCustomHeaders() map[string]string { | ||
| headers := map[string]string{} | ||
| for _, header := range c.CustomHeaders { | ||
| parts := strings.SplitN(header, ":", 2) | ||
| if len(parts) == 2 { | ||
| head := strings.TrimSpace(parts[0]) | ||
| value := strings.TrimSpace(parts[1]) | ||
| if head != "" { | ||
| headers[head] = value | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
getCustomHeaders silently ignores malformed --custom-headers entries (missing ':' or empty header name). This can lead to confusing behavior where the user thinks a header is being sent but it is dropped without any feedback. Consider validating the flag values and returning an error (or at least emitting a warning to stderr) when an entry is invalid.
| | Token Audience | `--api-audience` | `FGA_API_AUDIENCE` | `api-audience` | | ||
| | Store ID | `--store-id` | `FGA_STORE_ID` | `store-id` | | ||
| | Authorization Model ID | `--model-id` | `FGA_MODEL_ID` | `model-id` | | ||
| | Custom Headers | `--custom-headers` | `FGA_CUSTOM_HEADERS` | `custom-headers` | |
There was a problem hiding this comment.
The new Custom Headers row documents the flag/env/config key but doesn't describe the required value format or how to specify multiple headers in ~/.fga.yaml / FGA_CUSTOM_HEADERS. Adding a brief example (and aligning it with the actual parsing rules) would prevent user confusion.
Description
What problem is being solved?
Solves #669
How is it being solved?
Provide new
--custom-headersflag which allows to pass custom user-defined headers which are then included to every request to API.What changes are made to solve it?
Implemented new
--custom-headersoption by utilizing existingDefaultHeadersclient config field.References
Review Checklist
mainSummary by CodeRabbit
--custom-headersCLI flag. This repeatable flag accepts header values inname:valueformat, enabling users to include custom headers in requests.