fiaasco / borgbackup

Borg backup role
MIT License
16 stars 19 forks source link

Fix commands in templates and make them easier to read #26

Closed noplanman closed 4 years ago

noplanman commented 4 years ago

Closes #22

stroobl commented 4 years ago

This looks like a nice improvement. Are you already using the changed code? Seen the amount of changes in the script I would like to test it thoroughly.

noplanman commented 4 years ago

I'll fix up the tests, as they are expecting certain output which has changed by modifying the script.

@stroobl Yes, I'm using these in production with no problems.

noplanman commented 4 years ago

@stroobl @dverhelst Any idea why borgbackup_required needs to be defined explicitly even though it's set to true as a default value?

stroobl commented 4 years ago

@stroobl @dverhelst Any idea why borgbackup_required needs to be defined explicitly even though it's set to true as a default value?

I think that's required. Actually I just removed it (running from your branch) and the tests still pass for that part. Can you please revert all changes of variables in molecule yaml?

noplanman commented 4 years ago

@stroobl Those failing tests are due to the default value of borgbackup_required being set to False when undefined (via | default(False) in the prune script). The issue is what I described in my comment above, that the variable set in defaults/main.yml isn't being taken into account and it's undefined when the prune script is being created. That's why the tests fail, because the prune script doesn't have any hosts written into it, thus not finding the borg prune -v text that it's searching for.

For that reason, I've set the variable in the tests for the clients.

Actually I just removed it (running from your branch) and the tests still pass for that part.

Erm, well it fails for me, and clearly for you too, so not sure which part you meant with this 🤔

noplanman commented 4 years ago

Seems the failing build is due to some system issue, not the role itself.

@stroobl Can you restart the failed ones?

noplanman commented 4 years ago

@stroobl Is there anything else you'd like to comment on regarding this PR? 😇

stroobl commented 4 years ago

@noplanman there still two open remarks for molecule.yml that should be reverted if I'm not mistaken. I already tried to restart the build, but it still fails for one reason or another. I have to look into that (quite busy lately), but I don't think it's related to your changes.

noplanman commented 4 years ago

Have reverted those changes, they seem to have slipped in when merging master back 😬

Regarding the failing tests, it's strange, because they're only failing on Travis, local testing works perfectly. I'll investigate and see what's up 👌

noplanman commented 4 years ago

Seems to be some issue when installing multiple packages together, that some dependency problem occurs. I've tested by installing the redhat-lsb-core package first and then the others, which now works correctly. So it seems this is upstream, which has just popped up now and might just as well go away again in the future. You can check this successful build from my branch I'm busy working on.

Best we first get a fix for the tests in, then merge that into here, to make sure all tests work correctly.

Edit: PR done at #35