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

Creation of Server Settings Based On Environment Variables #300

Closed
wants to merge 19 commits into from

Conversation

deefdragon
Copy link

Using the server settings files generated from a clean start of the container, This commit adds 3 template json files, and one script file to resolve #299

This script file uses 4 functions to validate different parameter inputs (string, boolean, number, or empty) and makes sure that the input from the environment variable matches the required format. It also makes sure that there is a value to replace it with. If either of these cases is false, the default value is used.

Each file is created from the template in it's own function to allow for addition of or changing of keys in the future.

The current script is complete for the basic server settings, but not for the map settings or map-gen settings. The template keys have not yet been inserted into the template json file.

This is a request for review, to confirm that the required standards of error-proof and maintainable are met, and to discuss possible improvements.

@deefdragon deefdragon changed the title Creation of Server Settings Based On environment Variables Creation of Server Settings Based On Environment Variables Dec 25, 2019
@SuperSandro2000
Copy link
Member

Pipeline will be green if you don't merge master into master.

0.17/files/server-settings-template.json Outdated Show resolved Hide resolved
0.17/files/create-settings.sh Outdated Show resolved Hide resolved
0.17/files/create-settings.sh Outdated Show resolved Hide resolved
0.17/files/create-settings.sh Outdated Show resolved Hide resolved
0.17/files/create-settings.sh Show resolved Hide resolved
0.17/files/create-settings.sh Outdated Show resolved Hide resolved
@deefdragon
Copy link
Author

On the master to master, would it be best if I closed this and opened a new one? and would I merge a different branch on my end, or into a different branch on the factoriotools end?

@SuperSandro2000
Copy link
Member

On the master to master, would it be best if I closed this and opened a new one?

Don't know right now if you can change it. If yes change it if no just leave it like it is.

and would I merge a different branch on my end, or into a different branch on the factoriotools end?

You would merge a different branch on your end into factoriotools master.
Take a look at #267

@SuperSandro2000
Copy link
Member

Looking great!

@deefdragon
Copy link
Author

So, unless you see anything that needs to be changed, I will make the necessary changes to the docker-entrypoint.

@SuperSandro2000
Copy link
Member

BTW How do you plan to update the templates in the future?

@deefdragon
Copy link
Author

By hand comparison. there shouldn't be too many, if ANY changes between minor versions at this point and few between major versions, unless they do a full settings refactor or something.

@deefdragon
Copy link
Author

Right then, documentation created/updated, and control variables added.

The new method will not run unless GENERATE_SETTINGS_FILES is true, otherwise, the old method(copy from /opt/factorio/data) is used.

The value of FORCE_GENERATE_SETTINGS_FILES will determine if the settings are generated and replaced if the files exist already. Unless true, the generation will not run if the files exist already. This allows for the override of settings if needed by restarting the container.

Unless you see anything that needs to be added, changed, or removed, it should be good to merge.
I would like if someone else could sanity check me and also test it, but it should be good.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have time to test this right now. Can someone else verify that this does not break anything and works like it should?

@deefdragon
Copy link
Author

deefdragon commented Jan 24, 2020

Im going to add a commit to add this to the 0.18 folder as well, and will try to test as in depth as I can this weekend.

I dont like being the only one to test it, but at this point, its been long enough that I dont think I will have the mental auto-correct that I would get after just writing it.

@deefdragon
Copy link
Author

Closing for #304 . New branch name created accidentally. Running with it to allow for the checks to (hopefully) pass.

@deefdragon deefdragon closed this Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge in games-on-k8s/docker-factorio gen_config
2 participants