jammsen / docker-palworld-dedicated-server

Docker container to easily provision and manage Palworld Dedicated Server
https://hub.docker.com/r/jammsen/palworld-dedicated-server
MIT License
907 stars 157 forks source link

Added Server Settings auto/manual feature #96

Closed thejcpalma closed 7 months ago

thejcpalma commented 7 months ago

Key changes

thejcpalma commented 7 months ago

Might fix #73 due to setting the env var to manual it will stop the server config setup fucntion from modifying the settings file

MrMightyNighty commented 7 months ago

Wanted to do a PR myself but with a different approach:

Currently the files are validated each time the server has restarted/updated (https://github.com/jammsen/docker-palworld-dedicated-server/blob/1ea15dccca206e68147e1cba1cafee90873f4557/servermanager.sh#L17)

@jammsen Is it really necessary to validate the server on each update/restart? I think it would be better to put this behind a condition (should server always validate?), add an env variable and the config files shouldn't change at all (except someone added them as an env variable). This should be evaluated because I bet there will be server mods in the next months and validating the files could destroy any modded content.

jammsen commented 7 months ago

Hey @thejcpalma & @MrMightyNighty - Thanks for the contribution!

Key changes

  • Added feature to choose between env vars for the server settings or using the file directly;
  • Added all the env vars to the docker file so they are detected automatically on portainer and other container managers;
  • Spellchecked the project;

I like this PR very much! Its well describen and the ideas behind it are solid. Some things are basically refactoring and i just didnt had the time to look into that, so thank your very much for the time you invested to do it for me 🤣 especially the spell checking, im just a german dude who tries to talk and write in english sometimes better sometimes worse 👍

I totally agree with the "dumb/manual" mode idea. I even asked people multiple times if they wanted this, but no yes or nor clear answer. Im totally down to have one of theese modes, AS LONG as it respects Docker-Best Practices and the 12 Factor App idea. ( See https://github.com/jammsen/docker-palworld-dedicated-server/issues/95#issuecomment-1913307529 - https://github.com/jammsen/docker-palworld-dedicated-server/issues/29#issuecomment-1905586022 - https://github.com/jammsen/docker-palworld-dedicated-server/issues/10#issuecomment-1906496277 )

I would like to keep this PR and you as a contributor in the list after that of course, BUT i wanted to add some changes.

What i dont want to change (without a solid reason that is) is the "order" of the setings in Docker-Compose nor the Dockerfile. The Dockerfile still repesents the "Minimum Usage Scenraio" if you would like to call it that. Its basically the bare-minimum to run this. Docker-Compose is the "config everything your want" version and the sorting everywhere resembles the order of the DefaultPalWorldSettings.ini, so you dont have to Brain-Juggle your way through it, its just better customer-experience that way, except you have a valid reason to change that.

Im also not that sure what you mean in the List of Envs with "Game settings and Server settings" arent thoose values all used in the gameserver? What are you trying to seperate and explain here? @thejcpalma Please help me to understand.

The new table of contents make sense, thanks for that!

thejcpalma commented 7 months ago

Hello @jammsen !

First of all thanks for the swiftness in the response and the great work!

I like this PR very much! Its well describen and the ideas behind it are solid. Some things are basically refactoring and i just didnt had the time to look into that, so thank your very much for the time you invested to do it for me 🤣 especially the spell checking, im just a german dude who tries to talk and write in english sometimes better sometimes worse 👍

Always best to be clear, specially when there are different languages, so different understandings. I'm Portuguese so pardon me if I also missed something 😅 Glad I could help and seing your work this actively on this project and others (I follow the forest and SOTF repos too closely 😊) I'm happy to provide some help when needed.

I totally agree with the "dumb/manual" mode idea. I even asked people multiple times if they wanted this, but no yes or nor clear answer. Im totally down to have one of theese modes, AS LONG as it respects Docker-Best Practices and the 12 Factor App idea. ( See https://github.com/jammsen/docker-palworld-dedicated-server/issues/95#issuecomment-1913307529 - https://github.com/jammsen/docker-palworld-dedicated-server/issues/29#issuecomment-1905586022 - https://github.com/jammsen/docker-palworld-dedicated-server/issues/10#issuecomment-1906496277 )

I think it's a usefull feature in the way it allows granular control of the settings and disabling the config via environment variables so people can edit directly on the file, but it also might be hard to scale if more settings are added. It's a tricky theme to discuss honestly. About the best practices I can say it respects, but I also agree with @MrMightyNighty approach. If there's no changes there might not be a need to validate on each container start/restart.

What i dont want to change (without a solid reason that is) is the "order" of the setings in Docker-Compose nor the Dockerfile. The Dockerfile still repesents the "Minimum Usage Scenraio" if you would like to call it that. Its basically the bare-minimum to run this. Docker-Compose is the "config everything your want" version and the sorting everywhere resembles the order of the DefaultPalWorldSettings.ini, so you dont have to Brain-Juggle your way through it, its just better customer-experience that way, except you have a valid reason to change that.

Im also not that sure what you mean in the List of Envs with "Game settings and Server settings" arent thoose values all used in the gameserver? What are you trying to seperate and explain here? @thejcpalma Please help me to understand.

With this I agree, I just followed the PalWorld Settings Generator order on the README.md but I agree with you that it should mirror/copy the order of the original default file. Thanks for pointing that out 👌

Feel free to hop on a call with me or chat directly on some platform of your choosing for insight.

jammsen commented 7 months ago

Hello @jammsen !

First of all thanks for the swiftness in the response and the great work!

Thank you too, im just doing my thing to help 😄

I like this PR very much! Its well describen and the ideas behind it are solid. Some things are basically refactoring and i just didnt had the time to look into that, so thank your very much for the time you invested to do it for me 🤣 especially the spell checking, im just a german dude who tries to talk and write in english sometimes better sometimes worse 👍

Always best to be clear, specially when there are different languages, so different understandings. I'm Portuguese so pardon me if I also missed something 😅 Glad I could help and seing your work this actively on this project and others (I follow the forest and SOTF repos too closely 😊) I'm happy to provide some help when needed.

Awesome, thank you!

I totally agree with the "dumb/manual" mode idea. I even asked people multiple times if they wanted this, but no yes or nor clear answer. Im totally down to have one of theese modes, AS LONG as it respects Docker-Best Practices and the 12 Factor App idea. ( See #95 (comment) - #29 (comment) - #10 (comment) )

I think it's a usefull feature in the way it allows granular control of the settings and disabling the config via environment variables so people can edit directly on the file, but it also might be hard to scale if more settings are added. It's a tricky theme to discuss honestly. About the best practices I can say it respects, but I also agree with @MrMightyNighty approach. If there's no changes there might not be a need to validate on each container start/restart.

Yeah i mean more flexibility is always welcmome, i think that too. Also @MrMightyNighty OMG im so sorry i wanted to write something about this, but totally forgot it, im so sorry. The reason i do validate on every call is just that in the past SteamCMD around, FarmingSim19/22, ASE, TF and SOTF could be acting weird especially when Modded content is in there, because sometimes the cache for Mods on Steam was really weird and this solved having mods-problems as far as i could read in admin forum and tell from my own experience. If needed we could add an ENV for that too? Thats a way you can go with you 2? Im not hard-set on this, while i would always want this, to get the "latest-version" of Default-Configs in a Docker-Way to say.

What i dont want to change (without a solid reason that is) is the "order" of the setings in Docker-Compose nor the Dockerfile. The Dockerfile still repesents the "Minimum Usage Scenraio" if you would like to call it that. Its basically the bare-minimum to run this. Docker-Compose is the "config everything your want" version and the sorting everywhere resembles the order of the DefaultPalWorldSettings.ini, so you dont have to Brain-Juggle your way through it, its just better customer-experience that way, except you have a valid reason to change that.

Im also not that sure what you mean in the List of Envs with "Game settings and Server settings" arent thoose values all used in the gameserver? What are you trying to seperate and explain here? @thejcpalma Please help me to understand.

With this I agree, I just followed the PalWorld Settings Generator order on the README.md but I agree with you that it should mirror/copy the order of the original default file. Thanks for pointing that out 👌

Feel free to hop on a call with me or chat directly on some platform of your choosing for insight.

Sure add me on Discord - Nickname jammsen

miaulightouch commented 7 months ago

This feature is great for migrating from an existing server to Docker. I hope this can be merged ASAP.

However, I would like to have an option like 'skip.' Regardless of whether the PalWorldSettings.ini exists or not, I would skip any configuration process and start the server immediately.

thejcpalma commented 7 months ago

Fixed the conflicts due to the other PR #94 , seems good to go now 👌✨

thejcpalma commented 7 months ago

Sure add me on Discord - Nickname jammsen

Added you, I'm xplaypt on discord with dark magician avatar from yu-gi-oh 🧙🏿‍♂️

jammsen commented 7 months ago

Was merged, thanks for the contribution!

jammsen commented 7 months ago

This feature is great for migrating from an existing server to Docker. I hope this can be merged ASAP.

However, I would like to have an option like 'skip.' Regardless of whether the PalWorldSettings.ini exists or not, I would skip any configuration process and start the server immediately.

Should be like that