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

Added docker support #2455

Closed
wants to merge 10 commits into from
Closed

Added docker support #2455

wants to merge 10 commits into from

Conversation

YannickFricke
Copy link

@YannickFricke YannickFricke commented Dec 14, 2023

Description

  • Added the multi-staged Dockerfile
  • Added the docker-compose.yml file
    • It also includes a MongoDB service
  • Added the entrypoint for the Docker image

Issues fixed by this PR

Closes #2038
Closes #2250
Closes #2018

Type of changes

  • Bug fix
  • New feature
  • Enhancement
  • Documentation

Checklist:

  • My code follows the style guidelines of this project
  • My pull request is unique and no other pull requests have been opened for these changes
  • I have read the Contributing note and Code of conduct
  • I am responsible for any copyright issues with my code if it occurs in the future.

- Added the multi-staged Dockerfile
- Added the docker-compose.yml file
  - It also includes a MongoDB service
- Added the entrypoint for the Docker image
Copy link
Member

@KingRainbow44 KingRainbow44 left a 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.

@YannickFricke
Copy link
Author

YannickFricke commented Dec 15, 2023

it would be a bit of a hassle to maintain

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:

  1. Build the server from scratch
  2. Running the container via docker-compose (luckily the MongoDB adapter has a timeout of 30 seconds)
  3. cat the config to the host system

Sadly GrassCutter doesn't provide a default configuration file in the source code :/

is there a specific reason you can't generate a config using Java and reflection

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.

additionally, it would be nice to have an environment variable to change where the image gets its resources.

You mean the GIT repository? That would be doable :)

@YannickFricke
Copy link
Author

It also seems that there is another problem with the configuration file:

grafik

In the code it states that it is at version 13 - but the server only generates a config of version 4...

@YannickFricke
Copy link
Author

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.
@YannickFricke
Copy link
Author

YannickFricke commented Dec 15, 2023

@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 :)

@YannickFricke
Copy link
Author

The server is starting correctly in Docker (even with the MongoDB connection) - but I cant test it with the client (need to download it first with this slow connection here)

grafik

@KingRainbow44
Copy link
Member

how does this read from the traditional 'config.json' file?

@YannickFricke
Copy link
Author

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.

@KingRainbow44
Copy link
Member

in it's current state, i don't think this pr will be going through. dropping complete support for config.json would be a disaster for the many users which already use grasscutter.

@YannickFricke
Copy link
Author

in it's current state, i don't think this pr will be going through. dropping complete support for config.json would be a disaster for the many users which already use grasscutter.

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)?

@KingRainbow44
Copy link
Member

not everyone uses a script to start grasscutter, instead why can't you read from a config.json if it exists and set environment variables like that?

@YannickFricke
Copy link
Author

not everyone uses a script to start grasscutter, instead why can't you read from a config.json if it exists and set environment variables like that?

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.

@kashalls kashalls mentioned this pull request Mar 17, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants