-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
enhancement(http): unifying http query parameters #22242
base: master
Are you sure you want to change the base?
enhancement(http): unifying http query parameters #22242
Conversation
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.
No docs review required
@jszwedko Gentle ping :) |
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.
Nice! Thanks for this improvement @sainad2222 ! I think the code looks good. It is a bit unfortunate that the docs don't clearly show that query
can be a key/value or key/values. I expect users may end up confused. If you feel up to it, you could try to dig into the docs generation to see if you can adjust it, but I don't think that needs to block this since the current docs also don't really clearly show how the query
option is used.
I left a couple of other minor comments below.
@@ -175,7 +175,7 @@ pub struct ElasticsearchConfig { | |||
#[configurable(metadata(docs::advanced))] | |||
#[configurable(metadata(docs::additional_props_description = "A query string parameter."))] | |||
#[configurable(metadata(docs::examples = "query_examples()"))] |
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.
It seems like these examples aren't rendering properly on the docs, but that seems to be an existing issue so we don't need to block this.
However, could we add to the examples an example of specifying a single value? I think all existing examples in query_examples()
for all of the sources only show specifying an array of values.
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.
As an alternative, you can hand write a small "how it works" section on query params
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.
@@ -515,7 +515,7 @@ base: components: sources: http_client: configuration: { | |||
options: "*": { | |||
description: "A query string parameter and it's value(s)." | |||
required: true | |||
type: array: items: type: string: {} |
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.
Technically this should indicate that either a string
or [string]
can be used, but I'm not sure the current cue schema supports this (or if the code that generates the cue from the struct definitions does) 🤔
Summary
Unifying http query parameters while maintaining backward compatibility. Refer to this for approach
Change Type
Is this a breaking change?
How did you test this PR?
Does this PR include user facing changes?
Checklist
make check-all
is a good command to run locally. This check isdefined here. Some of these
checks might not be relevant to your PR. For Rust changes, at the very least you should run:
cargo fmt --all
cargo clippy --workspace --all-targets -- -D warnings
cargo nextest run --workspace
(alternatively, you can runcargo test --all
)Cargo.lock
), pleaserun
dd-rust-license-tool write
to regenerate the license inventory and commit the changes (if any). More details here.References
query
for HTTP-based components that support it #12073