-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Added docker support #2455
Added docker support #2455
Conversation
- Added the multi-staged Dockerfile - Added the docker-compose.yml file - It also includes a MongoDB service - Added the entrypoint for the Docker image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while I think generating the config from a JS script is good, it would be a bit of a hassle to maintain. is there a specific reason you can't generate a config using Java and reflection? additionally, it would be nice to have an environment variable to change where the image gets its resources.
I think the same about the configuration file - it would be far better if GrassCutter would use environment variables directly - so we could get rid of the workaround (even without having all the reflection stuff) :) My current approach was:
Sadly GrassCutter doesn't provide a default configuration file in the source code :/
For me particular I've never worked with gradle - so we would need another entrypoint where also GSON is loaded - so we could easily generate the config. But as said above - it would be far better if GrassCutter would use environment variables.
You mean the GIT repository? That would be doable :) |
Also if we would refactor GC to use environment variables - this would be a breaking change :) |
BREAKING CHANGE: This will make the config.json obsolete!
Since the ConfigContainer now uses environment variables by default, there is no need for the config generation script which does the same before launching the Docker container.
@KingRainbow44 Could you please verify the latest changes if they work for you? :) I've refactored the ConfigContainer to use environment variables instead of a file configuration :) The last TODO is only to refactor the Dockerfile to use an "ARG" for the resources URL - but thats a no-brainer :) |
Before the parameters for connecting to the MongoDB weren't read from the environment variables.
how does this read from the traditional 'config.json' file? |
It does not anymore - everything was converted to environment variables. Due to the lack of Docker supporting only mounting specific files (AFAIK) you cannot even mount the config into the container under a specific path. |
in it's current state, i don't think this pr will be going through. dropping complete support for |
What about reading out the config.json file (like before) and generating a script (.bat / .sh) based on the old configuration and then deleting the file (so the generated script wont be overwritten)? |
not everyone uses a script to start grasscutter, instead why can't you read from a |
Because it would bloat up the Docker image - you would need dependencies like jq and an extensive bash script which are only used once in the whole lifecycle. And we all know that data transformations (which would be needed for nested config keys) isn't that comfortable to maintain. |
Description
Issues fixed by this PR
Closes #2038
Closes #2250
Closes #2018
Type of changes
Checklist: