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

Adjust HAProxy's existance to log for console masters. #11433

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

Codixer
Copy link
Contributor

@Codixer Codixer commented Sep 23, 2024

This is a VERY minor change, so minimal but VERY useful for system administrators. (I am one, and this was so confusing).

Exact same PR concept as: PaperMC/Velocity#1436

@Codixer Codixer requested a review from a team as a code owner September 23, 2024 23:39
Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Generally, LGTM, however should be merged into the Add support for Proxy Protocol patch itself instead of a new patch.

Additionally, if you are moving things around anyway, this might be nicer to be right under the other logging in bind(), e.g. under the velocity native logs, just to keep everything neatly in one place.

@Codixer
Copy link
Contributor Author

Codixer commented Sep 25, 2024

Generally, LGTM, however should be merged into the Add support for Proxy Protocol patch itself instead of a new patch.

Additionally, if you are moving things around anyway, this might be nicer to be right under the other logging in bind(), e.g. under the velocity native logs, just to keep everything neatly in one place.

So I'm still new with patching and the way paper does it's changes, however. I assume I would have to edit the Add support for Proxy Protocol patch through the CONTRIBUTION instructions? And then put it together with the rest of the logging on bind, so it's not all the way at the bottom of the method?

@lynxplay
Copy link
Contributor

Yea exactly, you'd edit that patch as per CONTRIB instructions. If you have any more questions, feel 100% to just ask in paper-dev on discord, I am sure someone is gonna be around to help you with it.

Worst case I can also just quickly merge this for you if everything goes wrong 👍

@lynxplay lynxplay merged commit 627cc64 into PaperMC:master Sep 27, 2024
3 checks passed
@Codixer
Copy link
Contributor Author

Codixer commented Sep 27, 2024

@lynxplay so out of my own curiosity, what has been chanced in support of the PR? Some other files/patches where modified as well.

@Machine-Maker
Copy link
Member

The diff was moved to a different patch, a patch that added the support in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

3 participants