brahmlower / ansible-factorio

A role for creating Factorio servers
https://galaxy.ansible.com/bplower/factorio/
4 stars 6 forks source link

Update factorio version and fix service group. #10

Closed Tobsic closed 4 years ago

Tobsic commented 4 years ago

Hi, your role is really great, thanks a lot!.

This PR sets Factorio version 0.17.79 as default and validates the download using checksum. Now the service_user have the service_group as primary group (could be different before)

Sadly I'm not able to get the tests running. There was some changes to get the deployment done. But the service can't be started, because systemd is not runable in a docker container (without a lot of work I guess). Changes regarding this are on the fix-tests branch.

brahmlower commented 4 years ago

Hey @Tobsic! Thanks for the PR, and sorry for my late response here. This PR looks great, though I do have one question- It looks like the "Check factorio bin version exists" step was removed in this PR. That step was an attempt to avoid re-downloading the same file when the playbook is run multiple times. Did you find that that step caused problems or was unhelpful while using the playbook? I haven't used this playbook or kept up with the ansible ecosystem in a couple years, so maybe that step was breaking convention?

I'm cool with it either way, I just wanted to check on it before merging 🙂

brahmlower commented 4 years ago

Oh! I just learned that the get_url step won't redownload if the file already exists (described here), which does make that "check bin version exists" step redundant.

brahmlower commented 4 years ago

Sadly I'm not able to get the tests running

Yeah, look like they're a bit of a mess. I've got a branch in the works (PR #11) to switch to molecule so running tests should be easier in the future

brahmlower commented 4 years ago

@Tobsic I've just re-imported the role in galaxy, so your changes are live now 👍 🎉 Thanks again for the PR, and feel free to open another issue/PR if you find any other issues