Code review comment for ~oddbloke/cloud-init/+git/cloud-init:lp1843276

Revision history for this message
Dan Watkins (oddbloke) wrote :

On Mon, Sep 09, 2019 at 04:30:16PM -0000, Scott Moser wrote:
> sorry for round trips.

No worries, I'd like to get this right!

> Diff comments:
>
> > diff --git a/cloudinit/atomic_helper.py b/cloudinit/atomic_helper.py
> > index 587b994..8770193 100644
> > --- a/cloudinit/atomic_helper.py
> > +++ b/cloudinit/atomic_helper.py
> > @@ -23,6 +25,10 @@ def write_file(filename, content, mode=_DEF_PERMS,
> > try:
> > tf = tempfile.NamedTemporaryFile(dir=os.path.dirname(filename),
> > delete=False, mode=omode)
> > + LOG.debug(
> > + "Atomically writing to file %s (via temporary file %s) - %s: [%s]"
> > + " %s bytes/chars",
>
> %s formats terribly here for 'mode' :-(.
> >>> "%s %d" % (0o644, 9)
>
> You want '%o' for that, otherwise you'll get some confusing non-sense of '420' as the mode.

TIL about %o. Thanks! Changed.

> Then, since you have to change that anyway... can we use '%d' for the bytes/chars ?
>
> I realize i'm probably the worst offender here, but len(content) is guaranteed to be an int.

Also changed.

« Back to merge proposal