Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Expose HTTPHost property in HTTPClientConfig#645

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
jkroepke wants to merge3 commits intoprometheus:main
base:main
Choose a base branch
Loading
fromjkroepke:host

Conversation

@jkroepke
Copy link
Member

While I was integrate that feature ingrafana/alloy#698, it figure out that the Host property was removed in#597. Turns out that I forget something.

Since host is problematic with DockerSD, i choice the termhttp_host to follow the pattern fromhttp_headers

Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Copy link
Member

@bborehambboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Turns out that I forget something.

Please explain this further.

@bboreham
Copy link
Member

In#549 you addedHost toHTTPClientConfig andhost tohttpClientOptions.

Since only the latter was used, I removed the former. Now you adding it back, and hopefully using it.
Do we need both? Why?

@jkroepke
Copy link
MemberAuthor

Hey@bboreham,

I only need the property in HTTPClientConfig within the context of Grafana Alloy.

I have to apologize. When I started with Golang a year ago, I implemented the wrong side due to my lack of knowledge and missing tooling. I just replicated the userAgent logic, which misled me.

Looking at it now, I realize that the host property in httpClientOptions is incorrect.

If you agree, I can revert the host in httpClientOptions.

@bboreham
Copy link
Member

Thanks for the explanation. It's fine, everybody makes mistakes. Just easier for me to follow if I can see the path.

However we may prefer to deprecate the un-needed one rather than removing it immediately.
That way we can see if anyone else came to depend on it.

@KarstenSiemer
Copy link

Hi! Anything missing here?

@SuperQSuperQ requested a review frombborehamDecember 4, 2024 22:27
@SuperQ
Copy link
Member

Ping@jkroepke@bboreham, any issues to resolve here?

Signed-off-by: Jan-Otto Kröpke <github@jkroepke.de>
@jkroepke
Copy link
MemberAuthor

jkroepke commentedJan 21, 2025•
edited
Loading

@SuperQ from my understanding, the review asks for a more specific doc note which I applied now.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@SuperQSuperQSuperQ approved these changes

@ArthurSensArthurSensAwaiting requested review from ArthurSens

@bborehambborehamAwaiting requested review from bboreham

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jkroepke@bboreham@KarstenSiemer@SuperQ
[8]ページ先頭
©2009-2025 Movatter.jp