-
Notifications
You must be signed in to change notification settings - Fork 240
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 #304
base: master
Are you sure you want to change the base?
Conversation
Next time please don't merge master and rather rebase on master. |
I have done what testing I can at this point. Did find a few things. Unfortunately, I cant seem to get my router to stop randomizing the ports, so I cant test that you can still actually connect to a net game(though it DOES show up on the public games server). Will take a look at it again later, but I suspect that I wont be able to get very far. |
Did some more testing. still having issues with my firewall. I can host with the client, but not over docker. The base factorio-tools one or my changes. I can connect to both of them over lan however, and it does work as expected. The only thing that I had to add was the new setting that they added in .18. If someone could grab it and test that they can access a public game for me that would be great. I have an 18.9 testing version pushed to my dockerhub
(ps, sorry about the merge instead of rebase. Work muscle-memory.) |
@deef0000dragon1 I've started your docker image at the following address: 188.40.151.194:34197 This is the log returned by the container:
|
then | ||
mv "$CONFIG/map-gen-settings.json" "$CONFIG/map-gen-settings.json.$(date +%Y.%m.%d.%H.%M.%S)" | ||
fi | ||
|
||
if [[ ! -f $CONFIG/map-settings.json ]] | ||
if [[ -f $CONFIG/map-settings.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.
I think those changes are a mistake.
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.
Basing off of https://linuxize.com/post/bash-check-if-file-exists/ and that i was getting errors when I had it the other way.
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.
I don't know why it should error but right now it checks if the file does exist and then copies the default file but we want to do it when the file does not exist.
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.
This is the section that backs up an existing settings file (if you force the update of the settings via FORCE_GENERATE_SETTINGS_FILES). As such, if the file exists, we want to back it up. Lines 278-295 are the ones that you might be confusing these with. those are the ones that only move the generated settings file in if a file does not already exist.
dc642e9
to
0182693
Compare
See pull #300 for context. Created new PR to account for new branch name on my end.