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 global server config + remove defaultconfigs feature #9904

Draft
wants to merge 2 commits into
base: 1.20.x
Choose a base branch
from

Conversation

SrRapero720
Copy link
Contributor

@SrRapero720 SrRapero720 commented Apr 12, 2024

this PR adds global serverconfig option which places server config files in CONFIGDIR instead of worlddir
also removes defaultconfigs because

  1. nobody know how it works
  2. nobody uses it
  3. why? the only viable purpose is help modpackers to generate preferred default config files when these doesn't exist, but they already wrap generated config files, and the only reason to use that feature is for serverconfigs (and again, with the global server config option it has NO SENSE.

This was intended as a breaking change for 1.20.5+
is not humanly possible (without a lot of hackery) to keep compatibility.
Can be backported the global serverconfig doing a small check if file exists and if was a server config file to add manually the server.toml suffix and prevent duplicate filenames (if was even a problem) but is not something i will care about

@autoforge autoforge bot added Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. 1.20 labels Apr 12, 2024
@autoforge autoforge bot requested a review from a team April 12, 2024 02:31
@PaintNinja PaintNinja added the BreakingChange This request includes a change that would break compatibility with current versions of Forge. label Apr 12, 2024
@PaintNinja
Copy link
Contributor

Postponed until after first 1.20.5 build to allow time for proper discussion and review

Copy link
Member

@Jonathing Jonathing left a comment

Choose a reason for hiding this comment

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

I know we want to save discussion for this PR until 1.20.5, but I'd like to say that at its current state, this PR is a no-go. This entire implementation hinges on a Forge config value of checking if we want to use global server configs or not. The defaultconfig system allowed the game to use a pre-existing config file to base its world off of instead of creating an entirely clean config file, which is rather essential for modpack developers.

In my opinion, what this PR needs to do is rework the defaultconfig system where if a config is found in the global path, it is applied to a newly-created world. If we want to keep the config value of forcing using server configs from then global path, that is fine, but it should be disabled by default due to the modulative nature of server configs in the first place.

@Jonathing Jonathing added Regression This request demonstrates something missing in a newer version that was present in a prior version. Pending Rework and removed Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. labels Apr 14, 2024
@PaintNinja PaintNinja marked this pull request as draft April 15, 2024 13:50
@autoforge autoforge bot added the Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). label May 21, 2024
@autoforge
Copy link

autoforge bot commented May 21, 2024

@SrRapero720, this pull request has conflicts, please resolve them for this PR to move forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20 BreakingChange This request includes a change that would break compatibility with current versions of Forge. Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). Pending Rework Regression This request demonstrates something missing in a newer version that was present in a prior version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants