-
Notifications
You must be signed in to change notification settings - Fork 251
Expose config file locations as env vars #367
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
Conversation
Fixes #356 |
I am actually not sure why the automated tests are failing, can someone tell me what is wrong? I was able to build these locally without issue. |
Because the script is broken and missing a -t. |
CI should be fixed on master and because I cannot push to your branch you need to rebase on master. |
0.16/files/docker-entrypoint.sh
Outdated
@@ -59,4 +59,4 @@ $SU_EXEC /opt/factorio/bin/x64/factorio \ | |||
--rcon-port "$RCON_PORT" \ | |||
--rcon-password "$(cat "$CONFIG/rconpw")" \ | |||
--server-id /factorio/config/server-id.json \ | |||
"$@" | |||
"$@" |
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.
Missing final new line.
0.17/files/docker-entrypoint.sh
Outdated
@@ -95,4 +95,4 @@ else | |||
fi | |||
|
|||
# shellcheck disable=SC2086 | |||
exec $SU_EXEC /opt/factorio/bin/x64/factorio "${FLAGS[@]}" "$@" | |||
exec $SU_EXEC /opt/factorio/bin/x64/factorio "${FLAGS[@]}" "$@" |
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.
Missing final new line.
Also please never merge master in a PR and squash the commits together. |
Sorry this was the problem with rebasing off master, I ran into this issue then my existing pull request was already pointing to master. But yes, I will note that for the future. |
What's the status of this? It looks like there's a failing check for "stable", but I can't see any logs, and I'm not sure why these changes would fail for one version but not another? |
@@ -76,16 +76,41 @@ if [[ $GENERATE_NEW_SAVE == true ]]; then | |||
fi | |||
fi | |||
|
|||
if [[ -z "${FACTORIO_SERVER_ID_LOCATION:-}" ]]; then |
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.
To set default values you could change this if block to
: ${FACTORIO_SERVER_ID_LOCATION:=/factorio/config/server-id.json}
Example:
: ${test:=1 2} && echo $test
1 2
: ${test:=3 4} && echo $test
1 2
The :
is to make the command a nop, but the default value still gets set
@SQLJames we did a lot of changes and got rid of the version numbers in the folder structure. |
(Based on PR factoriotools#367, fixes factoriotools#356)
solved on the helm chart. SQLJames/factorio-server-charts#13 |
Fixes #356
In the factorio-server-charts, to allow us to specify the config values we are using configmaps, which creates the json file and mounts it to the proper location, however they are Read only files. This poses an issue when the server-id.json is needed to create a public server because it needs rw operations. I am currently looking into creating a sidecar to copy the files into the proper directory so we can properly rw to the directory.
But in my digging to resolve this issue, this solution did come up as a potential fix, so I had to ask if your project would be open to accepting a pull request that allows users to specify the location of config files as an ENV variable.