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

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

> I am curious
>
> 1. who is the runner when this library being called. (Ubuntu? Juju?) why they
> have root permission.

After looking into this, the script is called in cron.daily (or weekly/monthly depending on config) and it runs as root. Here is the relevant part in the code [1]. Which is why the copy command works without "sudo". I propose we can remove the sudo while copying from the tmp file.

```
$ cat /etc/cron.daily/charm-logrotated
#!/bin/bash
/usr/bin/sudo /usr/bin/juju-run logrotated/1 "/var/lib/juju/agents/unit-logrotated-1/.venv/bin/python3 /var/lib/juju/agents/unit-logrotated-1/charm/lib/lib_cron.py"
```

[1]: https://git.launchpad.net/charm-logrotated/tree/src/lib/lib_cron.py#n66

> 2. what's the different between f.flush and f.seek.

f.seek() would make the file pointer return to zero so that we can read contents from the start of the file. f.flush() is used to clear the internal buffer and put contents into the file explicitly before f.close() or reaching the end of the context manager (where the file is automatically closed). In this situation we could also use f.flush and push the contents to the tmp file before we read it, I just chose to pick f.seek here.

> 3. The logic looks like a magic if we want to copy the temp file. Is there
> better way to avoid the magic flush() or seek() function? I prefer not to use
> some implictly ability of flush() or seek() here. I prefer to make it just
> like a human do here.
>
> #1 create temp file
> #2 write data and close
> #3 copy file
> #4 delete temp file

In case you suggest to avoid using seek/flush, we can do it this way to make it more like how a human would do it:

```
with open(r"/tmp/crontab","w") as tmp_crontab:
    tmp_crontab.write(data)

try:
    cmd = ["cp", "/tmp/crontab", "/etc/crontab"]
    subprocess.run(cmd, check=True)

except subprocess.CalledProcessError as exception:
    hookenv.log(
        f"Writing to system crontab failed with error: {exception}",
        level=hookenv.ERROR,
    )
    raise
os.remove("/tmp/crontab")
```

« Back to merge proposal