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

Adding all game setting variables to ENV VAR #40

Closed gchai closed 7 months ago

gchai commented 7 months ago

For most of the variables, it was a guess as to what the data types were. Some added regex for things like the BanListURL as well as all the float values.

Adheared to the snake_case of existing variables

Also reordered variables to match the game file, to make it easier to edit and keep track.

gchai commented 7 months ago

References #10

MrMightyNighty commented 7 months ago

You should also change the README and/or add an example for the docker-compose so users are aware of this change :)

jammsen commented 7 months ago

i already wrote it in #10 but yeah same question

Hey @gchai - Thats some nice work, are you already on the readme for this too?

jammsen commented 7 months ago

@gchai @MrMightyNighty please have a look at #44 too.

gchai commented 7 months ago

What do you think is the best way to add info to the README? We can either:

I think the first option would be the best one, however I don't want to claim that I know what each env var does and that I got the data types and/or limits correct (What if one of the ints is actually capable of being a float?

jammsen commented 7 months ago

What do you think is the best way to add info to the README? We can either:

  • Enumerate all the options (Even though we don't know what it does)
  • Point the user to an example settings.ini file and say we support all of them (as of writing)
  • Point the user to the lines in servermanager.sh and say "For Power Users, you can have fine control over your settings here"

I think the first option would be the best one, however I don't want to claim that I know what each env var does and that I got the data types and/or limits correct (What if one of the ints is actually capable of being a float?

I also think the first option would be the most "customer-pov" looking solution, while surely beeing the most work 🤣

How about you post "the question" again over at #44 because i really want to best of both PR given out to you the people and the other solution has some charms too, also i think the group of people can discuss very good on this topic!

Also

(What if one of the ints is actually capable of being a float?

Fair point

jammsen commented 7 months ago

~~Again everyone read this here at #40 please jump over to #44 and try to help to make this 1 good solution. @gchai @MrMightyNighty @aaomidi @Lexiv - You contributors too please if you dont mind 😄~~

gchai commented 7 months ago

I added a draft of the table for all of the current variables and an update to README to point to it for "advanced users". Another option is to make it a collapsible section in the README.

MrMightyNighty commented 7 months ago

About documentation: While number 1 takes the "longest" time to write it is also the best one. Most of the params are mentioned on their tech guide. If there are params without a description the README should/could say something like "no info available". There is also a lazy method: Just link the user to the tech guide. Cons: If Pocket Pair adds new params the user will think "Nice I can just change this" and will be suprised that nothing changed.

About the implementations: I personally prefer this over #44. Shouldn't a .env work anyway with this implementation? With #44 there is no way a user can write their env variables inside the docker-compose if they prefer it that way.

jammsen commented 7 months ago

Seems like we discuss here now, so again my post over there was this

This looks very interesting and very according to the Docker-Best-Practice way. There is also another PR from @gchai under https://github.com/jammsen/docker-palworld-dedicated-server/pull/40 thats more focussed on very intensive script usage. Hmmm, both are very interesting and ALSO BOTH missing documentation about the "truck-load" of settings we would deliver 🤣 Can we all please stick the heads together and make 1 GREAT solution to this?

AND this

This solution is very Docker-Best-Practice way with using .env files to keep settings and secrets private. But its also fragile and very update heavy on the context of Pocketpair releasing a new config and we have to update this all the time and maybe the gameserver will not start until ALL peole update the config or the image or we fix the bug, because of missing params or failed validation or 100 other things we didnt thing about. The worst thing here can happen is that hundreds of server restart, get new server files and NONE is starting, which would be really bad and people surely would be sad or pissed off 🤣 and increase drastically traffic to the "issue-section" 🤣

The other solution is very scripting heavy and extends quite far the servermanager, but respects 100% the Default-Config that comes with the gameserver over SteamCMD no matter what happens. The worst thing here can happen is that the gameserver just starts and 1 config-param was not set/applied and its back to default-value then, BUT its always running. Which has a certain value for a "customer-perspective".

Any thoughts or problems i missed or problems with my analysis?

jammsen commented 7 months ago

@gchai in the draft there is every where a "{" inforont of the VARS

gchai commented 7 months ago

@gchai in the draft there is every where a "{" inforont of the VARS

Fixed, Regex issues!

Seems like we discuss here now, so again my post over there was this

Sorry about that! I didn't see the comments as I posted the updated PRs!

gchai commented 7 months ago

On one hand approach that #44 takes is much easier to maintain, on the other hand, has no error checking and it requires all of the settings parameters to be entered, not just the one that the user wants to change. On the other hand the way #40 handles env vars has more error checking, but is harder to maintain if they ever decide to add (or even worse, change) an entry.

As the game is marked as early access, it is possible that breaking changes will occur, not only to the settings file, but the save games. I think the decision is up to you on which approach is more appropriate.

MrMightyNighty commented 7 months ago

Seems like we discuss here now, so again my post over there was this

This looks very interesting and very according to the Docker-Best-Practice way. There is also another PR from @gchai under https://github.com/jammsen/docker-palworld-dedicated-server/pull/40 thats more focussed on very intensive script usage. Hmmm, both are very interesting and ALSO BOTH missing documentation about the "truck-load" of settings we would deliver 🤣 Can we all please stick the heads together and make 1 GREAT solution to this?

AND this

This solution is very Docker-Best-Practice way with using .env files to keep settings and secrets private. But its also fragile and very update heavy on the context of Pocketpair releasing a new config and we have to update this all the time and maybe the gameserver will not start until ALL peole update the config or the image or we fix the bug, because of missing params or failed validation or 100 other things we didnt thing about. The worst thing here can happen is that hundreds of server restart, get new server files and NONE is starting, which would be really bad and people surely would be sad or pissed off 🤣 and increase drastically traffic to the "issue-section" 🤣

The other solution is very scripting heavy and extends quite far the servermanager, but respects 100% the Default-Config that comes with the gameserver over SteamCMD no matter what happens. The worst thing here can happen is that the gameserver just starts and 1 config-param was not set/applied and its back to default-value then, BUT its always running. Which has a certain value for a "customer-perspective".

Any thoughts or problems i missed or problems with my analysis?

My comment was already building upon your post but since we already had a discussion here I didn't post it in #44 😅

jtmckay commented 7 months ago

Thought I'd put my 2 cents into the convo here: I like #44 because it doesn't require mapping/learning a new abstraction, and should allow for updates with only a .env file change. In my setup I pass through all of the env variables using a .env, and do not use docker-compose to set the actual values.

If #44 is more brittle, than that is something to consider. Happy just to have my PR considered.

Ultimately I am not maintaining this repo, so it doesn't matter much to me either way.

gchai commented 7 months ago

If https://github.com/jammsen/docker-palworld-dedicated-server/pull/44 is more brittle, than that is something to consider. Happy just to have my PR considered.

I think #40 more brittle in that any changes will break it (or be unsupported) whereas #44 is more flexible in that regard.

MrMightyNighty commented 7 months ago

Some things to consider:

Lexiv commented 7 months ago

Personally I don’t think it’s really necessary to modify every parameter in the config file through environment variables.

Bare minimum is fine. If someone wants to change more than that I’d point them to the config file and make them aware which values may be replaced by env vars when the container gets recreated.

jammsen commented 7 months ago

If #44 is more brittle, than that is something to consider. Happy just to have my PR considered.

I think #40 more brittle in that any changes will break it (or be unsupported) whereas #44 is more flexible in that regard.

I think its the opposite, because if that var isnt known, its still after an update there, because we copy on every start the default config, so the bare minimum is always working. If then the Script runs, it will either be changed or not.

Some things to consider:

Do you mean unsupported in a way that we have not yet the ability for a new var to be changed? Because everything else i think we covered with #40

Personally I don’t think it’s really necessary to modify every parameter in the config file through environment variables.

Bare minimum is fine. If someone wants to change more than that I’d point them to the config file and make them aware which values may be replaced by env vars when the container gets recreated.

Oh people are requesting it a lot, at least 7 requests i got on all channels.

Happy just to have my PR considered. Ultimately I am not maintaining this repo, so it doesn't matter much to me either way.

Of course you are considered, i think of this as a teamwork or community! Happy to have you to contribute!

Also .... Yeah i guess the final decision is down to me, i thought you guys would create PR 264 which solves everything or brainstorm the context of that PR, which you did. Its awesome thanks!

Well i guess we will go with #40 because its more stable then #44 ( @jtmckay SORRY 😭, but thanks a lot for the work ). The question remains, will the docker-compose examples be to long? Should we include a section about .env?

MrMightyNighty commented 7 months ago

Do you mean unsupported in a way that we have not yet the ability for a new var to be changed? Because everything else i think we covered with https://github.com/jammsen/docker-palworld-dedicated-server/pull/40

Yea that's what I meant :)

Well i guess we will go with https://github.com/jammsen/docker-palworld-dedicated-server/pull/40 because its more stable then https://github.com/jammsen/docker-palworld-dedicated-server/pull/44 ( @jtmckay SORRY 😭, but thanks a lot for the work ). The question remains, will the docker-compose examples be to long? Should we include a section about .env?

IIRC there was a docker-compose.yml in the repo right? That would probably be the best way so the README isn't crowded that much

jammsen commented 7 months ago

@gchai Hey buddy look at this https://dysoncheng.github.io/PalWorldSettingGenerator/setting.html It was mentioned here https://github.com/jammsen/docker-palworld-dedicated-server/issues/32#issuecomment-1907179741

Maybe that could help to get the readme shimmering 🏆

gchai commented 7 months ago

Updated the README.md with a link to the supported_vars.md document. Is the table clear enough that ExampleGameVar is not a real variable?

jammsen commented 7 months ago

IIRC there was a docker-compose.yml in the repo right? That would probably be the best way so the README isn't crowded that much

Yes there was, i wanted to get rid of it, because i like a good readme thats well structured and has a few, if not many, examples.

Updated the README.md with a link to the supported_vars.md document. Is the table clear enough that ExampleGameVar is not a real variable?

Need sleep first (01:07 here), i will take a look tomorrow and participate i promise! I want this PR to be done tomorrow!

jammsen commented 7 months ago

Ehhhhh ... huh ... im totally not sure why it doesnt show here, because i followed githubs own instructions ... but i spent a lot of hours and this and i pushed the changes of #40 to master - You can find it here https://github.com/jammsen/docker-palworld-dedicated-server/commit/8546c890a518c99ab9be13b35a876c01ddaa7bde#diff-3b41cd2eeb76ac64c4b71b9cc6b253b358ab737f50d54e333b631ca27357e98e

Please go ahead and test it, feedback is welcome @gchai @MrMightyNighty @jtmckay @Lexiv

aaomidi commented 7 months ago

Honestly this massive bash script is getting a bit unwieldy and harder and harder for me to reason about. Is there any interest in turning this into something like a go binary where we can write some unit tests, etc for this?

jammsen commented 7 months ago

Honestly this massive bash script is getting a bit unwieldy and harder and harder for me to reason about. Is there any interest in turning this into something like a go binary where we can write some unit tests, etc for this?

I dont know any Go yet. Yeah the shell-script is massive, but its is what the community requested, to setup all vars.

jammsen commented 7 months ago

Also @aaomidi have you tested it?

jammsen commented 7 months ago

Feel free to give feedback here, but im closing this for status update.

aaomidi commented 7 months ago

I've not yet tested this, I was mainly reading the scripts first before I got into testing it.

Cheers! Let me know what I can do to help out here. Thank you so much for this repo @jammsen :)

jammsen commented 7 months ago

Let me know what I can do to help out here

What do you mean by that exactly or how do you mean this?

Cheers! Let me know what I can do to help out here. Thank you so much for this repo @jammsen :)

No problem 👍

natebluehooves commented 7 months ago

Personally I don’t think it’s really necessary to modify every parameter in the config file through environment variables.

Bare minimum is fine. If someone wants to change more than that I’d point them to the config file and make them aware which values may be replaced by env vars when the container gets recreated.

just a heads up: said config file is rewritten on server startup due to steam file validation iirc. I believe the reason players need/want everything editable via environment variable is for this reason.

gchai commented 7 months ago

It works great for me! Cheers!

Ehhhhh ... huh ... im totally not sure why it doesnt show here, because i followed githubs own instructions ... but i spent a lot of hours and this and i pushed the changes of https://github.com/jammsen/docker-palworld-dedicated-server/pull/40 to master

I see you wanted to make some changes, I think the "correct" way to do this would be to merge this into the develop branch, make your changes on top of that, then merge it into master to preserve git commit history

jammsen commented 7 months ago

Personally I don’t think it’s really necessary to modify every parameter in the config file through environment variables. Bare minimum is fine. If someone wants to change more than that I’d point them to the config file and make them aware which values may be replaced by env vars when the container gets recreated.

just a heads up: said config file is rewritten on server startup due to steam file validation iirc. I believe the reason players need/want everything editable via environment variable is for this reason.

Well you both are right in your own individual way 👍 The file /palworld/DefaultPalWorldSettings.ini comes out of the box from Palworld itself. Yes it is updated every time steamcmd does something because i have +validation active. This gives me a "always-working-default-config". I like that and i totally respect that. Thats why my solution combines this with copying it to the save-location and changing only things people added to it. A lot of people requested chaning "this 1 or 2 vars" and even more, wanted total control, again i understand and i respect that, as an admin i would like that too.

The bare-minimum of config, for me personally is defined in the Dockerfile, you can see if you compare the docker-compose examples with the Dockerfile, its not the same. Thats on purpose. Hopefully that wont be a bug in the long run 😎

Thanks for alle the help you guys.

jammsen commented 7 months ago

It works great for me! Cheers!

Ehhhhh ... huh ... im totally not sure why it doesnt show here, because i followed githubs own instructions ... but i spent a lot of hours and this and i pushed the changes of #40 to master

I see you wanted to make some changes, I think the "correct" way to do this would be to merge this into the develop branch, make your changes on top of that, then merge it into master to preserve git commit history

Ah i see, im so used to administration of Bamboo and Bitbucket DC and i teach/use/admin both of theese, sometimes Github can feel a bit "foreign" and im searching a missing function in my mind, sorry for that @gchai