Snowiiii / Pumpkin

Empowering everyone to host fast and efficient Minecraft servers.
https://snowiiii.github.io/Pumpkin/
MIT License
3.25k stars 113 forks source link

feat: add envvar for config file paths #323

Open kekrs opened 1 day ago

kekrs commented 1 day ago

Description

Docker or Kubernetes deployments would need a method to explicitly set configuration file paths. This PR adds functionality to set these.

Testing

Checklist

Things need to be done before this Pull Request can be merged.

Yimura commented 1 day ago

Can you give me a scenario in which this is required? This env var only applies to the path inside of the docker container and I don’t see a reason for why this needs to be configurable.

I’ve already made the effort in #255 to give the deployer all the required flexibility, I’m trying to understand and compare this configurability with other docker containers to understand your reasoning and I can’t.

Sure you’re saying for “what” it’s useful in this PR, just not “why” it’s useful. This is lacking the motivation to convince the main contributor, especially since he may not be as experienced in Docker and Kubernetes.

kekrs commented 1 day ago

Sure you’re saying for “what” it’s useful in this PR, just not “why” it’s useful. This is lacking the motivation to convince the main contributor, especially since he may not be as experienced in Docker and Kubernetes.

Most sensible scenario for me is when you want to store the configuration away from the data. Right now it's not an issue since we don't have world saving yet. In more professional deployments you want to store the configuration in a version-controlled store, which can be accessed by the process only in ReadOnly mode. Good example here is Kubernetes ConfigMap. The data is where we store our state, and configuration shouldn't be treated as such.

Using the current WORKDIR and mount the volume there is absolutely fine for home or small-scale use cases.

EDIT: For the completeness sake: Minecraft currently mixing config with state, and it is a bad practice.

Yimura commented 1 day ago

So a possible scenario for this would be (just a docker compose as an example):

services:
  pumpkin:
    image: snowiiii/pumpkin:1.21-latest
    environment: 
      - CONFIG_PATH=/config/configuration.toml # I understand these keys are wrong though I prefer to be specific to avoid conflicts with future environment variables
      - FEATURES_PATH=/config/features.toml 
    ports:
      - 25565:25565
    volumes:
      - config_volume:/config:ro
      - pumpkin_data_volume:/pumpkin

volumes:
  config_volume:
    external: true
  pumpkin_data_volume:

Wouldn't you be able to mount those files directly under /pumpkin/features.toml and /pumpkin/configuration.toml? Since these more elaborate deployment environments are usually hosted on Linux, this should be possible.

kekrs commented 1 day ago

Wouldn't you be able to mount those files directly under /pumpkin/features.toml and /pumpkin/configuration.toml? Since these more elaborate deployment environments are usually hosted on Linux, this should be possible.

In a nutshell your example is correct. However:

    environment: 
      - CONFIG_PATH=/pumpkin/config/configuration.toml
      - FEATURES_PATH=/pumpkin/config/features.toml 
    volumes:
      - config_volume:/pumpkin/config:ro
      - pumpkin_data_volume:/pumpkin

its possible to do this as well. But to be frank, I wouldn't trust it with my life. This example is bad practice though.

In a hypothetical scenario there's a software/user/config error during the mounting process, we'll end up with two different and possibly diverging configurations. When modding or other world changing settings are introduced, this could lead to inconsistencies, even world corruption.

Let's say I have a configuration that alters worldgeneration. I forgot to mount the file to it's correct place. The software generates an empty "vanilla" configuration, and starts up, now it will generate chunks that way.

Another example is I have custom authentication configured. File fails to mount and now unauthorized players can join.