Skip to content
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

Merged

Conversation

cmbrad
Copy link
Contributor

@cmbrad cmbrad commented Nov 14, 2023

PR Description

Updates discovery.ec2 to allow specifying HTTPClientConfig fields such as proxy_url. This allows discovery.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

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@cmbrad cmbrad requested a review from a team as a code owner November 14, 2023 11:07
@spartan0x117
Copy link
Contributor

👋 Thanks for the contribution! The docs changes would go in docs/sources/flow/reference/components/discovery.ec2.md, if you could add those.

You would probably want to add a new block section with:

{{< docs/shared lookup="flow/reference/components/http-client-config-block.md" source="agent" version="<AGENT VERSION>" >}}

And you don't need to rewrite the contents 😃

@cmbrad
Copy link
Contributor Author

cmbrad commented Nov 15, 2023

Thanks for the pointer! By the look of the existing codebase (ie docs/sources/flow/reference/components/remote.http.md) that shared block works well when the options are in a separate client block - but currently I think this implementation just squashes arguments rather than using a separate block (to match the majority of other discovery component implementations).

Instead I chose to match docs/sources/flow/reference/components/discovery.consul.md (and other similar ones) which put a few select arguments directly in-line. Let me know what you think

Copy link
Contributor

@spartan0x117 spartan0x117 left a 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

@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Nov 21, 2023
@clayton-cornell
Copy link
Contributor

@spartan0x117 Looks OK to me. I haven't looked into the conflicts.

Copy link
Member

@tpaschalis tpaschalis left a 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!

@tpaschalis tpaschalis enabled auto-merge (squash) December 8, 2023 14:22
@tpaschalis tpaschalis merged commit e7e8c00 into grafana:main Dec 8, 2023
10 checks passed
BarunKGP pushed a commit to BarunKGP/grafana-agent that referenced this pull request Feb 20, 2024
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow customising HTTPClientConfig in discovery.ec2 component
4 participants