Skip to content

Add configuration interface to expose certain config values #12273

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

NonSwag
Copy link
Contributor

@NonSwag NonSwag commented Mar 11, 2025

Previously, people (I) have been using the spigot and paper file configurations to determine whether the server is configured for proxy online mode
Now the spigot interface got deprecated for good reasons, but there is no alternative

image

This commit introduces methods to check the online mode and enablement status of Velocity and BungeeCord. These changes allow plugins to easily query proxy-related server configurations.

I would like to have some input on the method names since Bukkit uses getOnlineMode
Also, I wasn't really sure on whether to return bungee/velocity online mode true only if the server is actually using bungee or velocity

This commit introduces methods to check the online mode and enablement status of Velocity and BungeeCord. These changes allow plugins to easily query proxy-related server configurations.
@NonSwag NonSwag requested a review from a team as a code owner March 11, 2025 18:22
@github-project-automation github-project-automation bot moved this to Awaiting review in Paper PR Queue Mar 11, 2025
@github-project-automation github-project-automation bot moved this from Awaiting review to Awaiting final testing in Paper PR Queue Mar 12, 2025
@Machine-Maker
Copy link
Member

I wouldn't be immediately opposed to a new interface where select values from the configuration can be exposed more directly. I wouldn't want even more methods on Bukkit/Server for this though. I think there are probably these and several more values which we can consider stable enough to expose in the API.

@NonSwag
Copy link
Contributor Author

NonSwag commented Mar 12, 2025

I wouldn't be immediately opposed to a new interface where select values from the configuration can be exposed more directly. I wouldn't want even more methods on Bukkit/Server for this though. I think there are probably these and several more values which we can consider stable enough to expose in the API.

I like that idea, let me cook something up

moved all previously added methods to the new server config interface
@NonSwag
Copy link
Contributor Author

NonSwag commented Mar 12, 2025

I've added a new interface ServerConfiguration

@NonSwag NonSwag requested a review from kennytv March 12, 2025 19:38
@NonSwag NonSwag changed the title Add proxy mode and proxy type checks to the API Add configuration interface to expose certain config values Mar 12, 2025
@Warriorrrr Warriorrrr added type: feature Request for a new Feature. scope: api labels Mar 17, 2025
@@ -0,0 +1,38 @@
package io.papermc.paper.configuration;

public interface ServerConfiguration {
Copy link
Member

Choose a reason for hiding this comment

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

Probably wanna note here that this class doesn't aim to expose every single config option in existance, only select ones.

Choose a reason for hiding this comment

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

I feel like "selected critical settings." still makes it rather vague. Like why exactly are those critical? Are they critical for the server? The plugin dev? Both?

Maybe mentioning why they are considered critical would be beneficial here.

@NonSwag NonSwag requested review from mbax, Warriorrrr and kennytv March 24, 2025 15:37
@NonSwag NonSwag requested a review from Warriorrrr March 31, 2025 17:14
@electronicboy
Copy link
Member

While I approve of the general concept of this, I'm not entirely fond about having highly specific stuff exposed if the context starts relating into stuff like security aspects, etc; i.e. what happens if we add a brand new forwarding mechanism that has its own option, does that get merged into the existing one or do we have an entire open hole? Do we really need this level of accessibility?

Copy link

@Andre601 Andre601 left a comment

Choose a reason for hiding this comment

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

Some comments regarding the comments.

@@ -0,0 +1,38 @@
package io.papermc.paper.configuration;

public interface ServerConfiguration {

Choose a reason for hiding this comment

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

I feel like "selected critical settings." still makes it rather vague. Like why exactly are those critical? Are they critical for the server? The plugin dev? Both?

Maybe mentioning why they are considered critical would be beneficial here.

NonSwag added 2 commits April 13, 2025 20:21
# Conflicts:
#	paper-api/src/main/java/org/bukkit/Bukkit.java
#	paper-server/src/main/java/org/bukkit/craftbukkit/CraftServer.java
@NonSwag
Copy link
Contributor Author

NonSwag commented Apr 13, 2025

While I approve of the general concept of this, I'm not entirely fond about having highly specific stuff exposed if the context starts relating into stuff like security aspects, etc; i.e. what happens if we add a brand new forwarding mechanism that has its own option, does that get merged into the existing one or do we have an entire open hole? Do we really need this level of accessibility?

As written in the ServerConfiguration docs, it only aims to provide the most important settings
In my opinion there is nothing else that is really important to be exposed here as well at the moment
but I think this is better discussed internally

"This interface doesn't aim to cover every possible server configuration option but focuses on selected critical settings."

@electronicboy
Copy link
Member

these settings are important why? What is the merit of exposing them individually rather than exposing the underlying concepts behind them?

@NonSwag
Copy link
Contributor Author

NonSwag commented Apr 13, 2025

I don't think I can follow. What do you mean by "the underlying concepts"?
The problem is that it isn't possible to check whether the server is configured for online mode and changing the Server#getOnlineMode impl seems iffy
So exposing them here/at all is kinda critical

@NonSwag NonSwag requested review from Warriorrrr and Andre601 April 13, 2025 18:54
@electronicboy
Copy link
Member

Yes, so, isProxyOnline mode makes sense, individual bungee/velocity forwarding mode ones, I'm not sure about

@NonSwag
Copy link
Contributor Author

NonSwag commented Apr 13, 2025

You are right, thinking about it, I don't see a real upside in exposing them.
I think I am going to remove them

@NonSwag
Copy link
Contributor Author

NonSwag commented Apr 14, 2025

I removed the forwarding mode related methods
I would suggest for the distant future, moving stuff like getPort and getIp and other methods from the Bukkit interface to this class to unclutter that mess of a class

@Andre601
Copy link

Yes, so, isProxyOnline mode makes sense, individual bungee/velocity forwarding mode ones, I'm not sure about

It could be useful in case there would be Velocity-only features to use through Paper in the future.
Tho, that could also be checked via some message channel stuff I suppose.

@lynxplay
Copy link
Contributor

If there are such features in the future, we can expand this type 😉

@electronicboy
Copy link
Member

I mean, the two major things I'd imagine plugins might want to see on this front are "Are we in online mode?" and "Are we behind a proxy?" disambiguation is complex, I generally wouldn't want us to be in a state where we add support for some new proxy mode and now every plugin relying on this API to check that is broken

@lynxplay lynxplay moved this from Awaiting final testing to PR Party candidate in Paper PR Queue Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: api type: feature Request for a new Feature.
Projects
Status: PR Party candidate
Development

Successfully merging this pull request may close these issues.

8 participants