-
-
Notifications
You must be signed in to change notification settings - Fork 173
feat: add support for all retrievers in cli #4025
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
base: main
Are you sure you want to change the base?
feat: add support for all retrievers in cli #4025
Conversation
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
Signed-off-by: deamondev <[email protected]>
✅ Deploy Preview for go-feature-flag-doc-preview canceled.
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4025 +/- ##
==========================================
+ Coverage 83.92% 84.04% +0.11%
==========================================
Files 137 137
Lines 6813 6968 +155
==========================================
+ Hits 5718 5856 +138
- Misses 884 897 +13
- Partials 211 215 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
||
func (s *Retriever) SetDownloader(downloader DownloaderAPI) { | ||
s.downloader = downloader | ||
} |
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.
Just to be sure, why do you need this function?
You can probably set the downloader
directly in the struct?
/gemini review |
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.
Code Review
This pull request significantly enhances the CLI by adding support for all retrievers, which is a great feature. The implementation introduces a --check-mode
for inspecting retriever configurations, which is very useful for debugging.
My review focuses on improving documentation clarity, fixing a parsing bug, and some minor code cleanup. I've identified a few issues in the README.md
that could confuse users, a bug in the HTTP header parsing logic, and some opportunities for code refinement in the tests and command implementation.
Overall, this is a solid contribution. Addressing the feedback will make the new CLI features more robust and easier to use.
func parseHTTPHeaders() map[string][]string { | ||
result := make(map[string][]string) | ||
for _, h := range headers { | ||
parts := strings.SplitN(h, "=", 2) | ||
if len(parts) != 2 { | ||
parts = strings.SplitN(h, ":", 2) | ||
} | ||
if len(parts) != 2 { | ||
continue | ||
} | ||
key := strings.TrimSpace(parts[0]) | ||
val := strings.TrimSpace(parts[1]) | ||
result[key] = append(result[key], val) | ||
} | ||
return result | ||
} |
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.
The current parsing logic for HTTP headers can lead to incorrect key-value pairs if a header value contains an =
character. The logic attempts to split by =
before splitting by :
. It should be the other way around, as :
is the standard separator for HTTP headers. For example, a header like Some-Header: key=value
would be incorrectly parsed. Swapping the order of checks will fix this.
func parseHTTPHeaders() map[string][]string { | |
result := make(map[string][]string) | |
for _, h := range headers { | |
parts := strings.SplitN(h, "=", 2) | |
if len(parts) != 2 { | |
parts = strings.SplitN(h, ":", 2) | |
} | |
if len(parts) != 2 { | |
continue | |
} | |
key := strings.TrimSpace(parts[0]) | |
val := strings.TrimSpace(parts[1]) | |
result[key] = append(result[key], val) | |
} | |
return result | |
} | |
func parseHTTPHeaders() map[string][]string { | |
result := make(map[string][]string) | |
for _, h := range headers { | |
parts := strings.SplitN(h, ":", 2) | |
if len(parts) != 2 { | |
parts = strings.SplitN(h, "=", 2) | |
} | |
if len(parts) != 2 { | |
continue | |
} | |
key := strings.TrimSpace(parts[0]) | |
val := strings.TrimSpace(parts[1]) | |
result[key] = append(result[key], val) | |
} | |
return result | |
} |
|------------|---------------------------------------------------------------------|---------| | ||
| `--url` | URL of the remote flag configuration file | "" | | ||
| `--method` | HTTP method to access your configuration file on HTTP | "" | | ||
| `--body` | Http body to access your configuration file on HTTP | GET | |
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.
The default value for --body
is documented as GET
, which is incorrect. GET
is the default for the --method
flag. The default for --body
should be an empty string (""
).
| `--body` | Http body to access your configuration file on HTTP | GET | | |
| `--body` | Http body to access your configuration file on HTTP | "" | |
| `--repository-slug` | | "" | | ||
| `--branch` | Name of the repository | "" | | ||
| `--auth-token` | Authentication token to access your configuration file | "" | | ||
| `--github-token` | Authentication token to access your configuration file on GitHub<br/> (⚠️ deprecated, use `--auth-token` instead) | "" | | ||
| `--path` | Path to the remote flag configuration file inside github repository | "" | | ||
|
||
#### GitLab | ||
|
||
| Flag | Description | Default | | ||
|---------------------|---------------------------------------------------------------------|---------| | ||
| `--base-url` | Base URL of your configuration file on Gitlab | "" | | ||
| `--repository-slug` | Name of the repository | "" | | ||
| `--branch` | Name of the repository | "" | | ||
| `--path` | Path to the remote flag configuration file inside gitlab repository | "" | | ||
|
||
#### BitBucket | ||
|
||
| Flag | Description | Default | | ||
|---------------------|------------------------------------------------------------------------|---------| | ||
| `--base-url` | Base URL of your configuration file on BitBucket | "" | | ||
| `--repository-slug` | Name of the repository | "" | | ||
| `--branch` | Name of the repository | "" | | ||
| `--path` | Path to the remote flag configuration file inside bitbucket repository | "" | |
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.
There are several documentation issues in the tables for Git-based retrievers (GitHub, GitLab, BitBucket):
- GitHub (
--repository-slug
): The description is missing. - GitHub, GitLab, BitBucket (
--branch
): The description is incorrectly 'Name of the repository'. It should describe the branch. - GitLab, BitBucket (
--repository-slug
): The description is incorrectly 'Name of the repository'. It should describe the repository slug.
Please update these descriptions for clarity. For example, you could use 'Repository slug (e.g., owner/repo)' and 'Git branch name'.
"column", nil, "Postgres column mapping of your configuration file on Postgres (may be repeated)") | ||
_ = evaluateCmd.Flags().MarkDeprecated("github-token", "Use auth-token instead") | ||
_ = evaluateCmd.Flags().MarkDeprecated("config", "Use path instead") | ||
_ = evaluateCmd.Flags() |
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.
expectedResult: "testdata/res/check-gcs.json", | ||
}, | ||
{ | ||
name: "should return configuration of GCS retriever when using check-mode", |
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.
func Test_evaluate_Evaluate(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
evaluate evaluate |
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.
Description
I added new flags related to other kind of retrievers. Based on them the new retriever is being spawned (but see #4024).
There is also new
--check-mode
flag which when present would only dump the configuration of retriever being used to performevaluate
call. It also served me while testing some multi-flags (like http's--header
) because there is some kind of parsing logic atevaluate_cmd
layer.The
redis
retriever is not supported because of #4023. We may of course hack it and provide empty output when-kind=redis
but I think the refactor ofRetrieverConf
struct would benefit more.Closes issue(s)
Resolve #3876
Checklist
README.md
and/website/docs
)