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

fix: change headless service to gRPC and expose 9094 TCP #494

Merged
merged 25 commits into from
Apr 9, 2024

Conversation

locmai
Copy link
Contributor

@locmai locmai commented Apr 3, 2024

What this PR does

Follow-up the work of @AlexandreRoux from #435 and related to #420 to run the helm-docs for the README.md

This PR corrects the alertmanager headless service with exposing gRPC port instead of HTTP.

I tested the Helm chart and I can't reproduce the EOF issue.

humblebundledore and others added 18 commits February 16, 2023 10:03
Signed-off-by: AlexandreRoux <[email protected]>
Signed-off-by: Loc Mai <[email protected]>
Signed-off-by: Loc Mai <[email protected]>
Comment on lines 22 to 23
name: http-metrics
targetPort: http-metrics
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should inform the user about this breaking chance in CHANGELOG.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the CHANGELOG.md to make this clearer. Or do you think we should add a better indication that this may break if users were using the http-metrics port for their own usage? Similar to the old PR we have here #169

And by breaking - do you mean we should make it to v3 (too much I think?) or a line in the CHANGELOG should be good enough?

Copy link
Collaborator

@nschad nschad Apr 8, 2024

Choose a reason for hiding this comment

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

Hmmm. I don't have a strong opinion about it. Technically it should be a major release because it will break if somebody depends on it (somehow).

And to be honest so #169 should have been.

CC: @kd7lxl Do you have an opinion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just not touch the http-metrics port, so that there is no breaking change. Adding gRPC has nothing to do with the http-metrics port.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good. Let me update that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Signed-off-by: Loc Mai <[email protected]>
Signed-off-by: Loc Mai <[email protected]>
Signed-off-by: Loc Mai <[email protected]>
Signed-off-by: Loc Mai <[email protected]>
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Loc Mai <[email protected]>
@nschad nschad requested a review from kd7lxl April 8, 2024 09:25
Signed-off-by: Loc Mai <[email protected]>
CHANGELOG.md Outdated
Comment on lines 5 to 6
* [CHANGE] Alertmanager: Add `grpc` port #494
* [CHANGE] Alertmanager: Expose 9094 TCP and UDP for gossip cluster #494
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be ENHANCEMENT.

Signed-off-by: Loc Mai <[email protected]>
@nschad nschad merged commit 1ad5dfb into cortexproject:master Apr 9, 2024
2 checks passed
@locmai locmai mentioned this pull request Apr 12, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants