-
Notifications
You must be signed in to change notification settings - Fork 488
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
Add HTTPClientConfig to discovery.ec2 and add a simple test #5767
Add HTTPClientConfig to discovery.ec2 and add a simple test #5767
Conversation
👋 Thanks for the contribution! The docs changes would go in You would probably want to add a new block section with:
And you don't need to rewrite the contents 😃 |
Thanks for the pointer! By the look of the existing codebase (ie Instead I chose to match |
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.
LGTM! Pending review from @clayton-cornell I can get this merged
@spartan0x117 Looks OK to me. I haven't looked into the conflicts. |
Signed-off-by: Paschalis Tsilias <[email protected]>
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.
I've resolved the conflicts and will merge this as per the previous feedback.
Thanks @cmbrad!
Co-authored-by: Paschalis Tsilias <[email protected]>
PR Description
Updates
discovery.ec2
to allow specifying HTTPClientConfig fields such asproxy_url
. This allowsdiscovery.ec2
to work behind a corporate proxy.Which issue(s) this PR fixes
Fixes #5766
Notes to the Reviewer
There are currently no tests - I've added a very basic test. It's not entirely clear to me how/where to update the docs. grafana/alloy#530 seems to relate to this.
It appears various discovery components either implement this functionality by injecting the whole
HTTPClientConfig
or just specific fields. I've gone with the simple implementation here.PR Checklist