Code review comment for ~dashmage/charm-logrotated:bug/2017798-update-cron-daily-location

Revision history for this message
Ashley James (dashmage) wrote (last edit ):

> ditto, 0o755 or 0o644 ? , 0x744 seems weird

Yeah, I agree. I've made this `0o755` now.

> It seems we add function argument `target` is just because we need to test it.
But the value will never change in real case.
I will suggest to remove it and mock the `open` syntax.

I've now modified the unit test so that it mocks the open context manager instead of relying on the `target` argument which would never change. Good suggestion!

> Please provide the final result we write into /etc/crontab for the verificiation

As part of the updated unit tests, the final results are now specified and verified.

« Back to merge proposal