Code review comment for ~goneri/cloud-init:freebsd_mount_sync

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

On Tue, Mar 26, 2019 at 02:29:51PM -0000, Gonéri Le Bouder wrote:
> mount_cb's rw flag is always False. This is actually what leads my to
> thing the `sync` flag is pointless. I'm convinced we can drop the two
> parameters.

I wouldn't be opposed to dropping the rw argument to mount_cb if it
isn't used anywhere, but that isn't what you had proposed thus far. :)

> In addition, the current code base relies on blkid which is Linux
> specific. To get cloud-init to work on FreeBSD, we need to mock blkid.

cloud-init does already work on FreeBSD for some data sources. I
certainly agree that this particular data source needs some work. :)

> In addition, there is no Linux 'distro' beside FreeBSD.

I don't really know what this means; FreeBSD isn't a Linux 'distro', and
there are certainly operating systems other than GNU/Linux and FreeBSD.
(Also, platform.system() reads from uname data, so it could feasibly be
any string.)

> So I doubt such change will impact anyone.

I also doubt that such a change would impact anyone, but "it will
probably be fine" is (annoyingly, I agree) not the standard we have to
hold ourselves to for a piece of software that is fundamentally
important to boot in so many environments, and that is very difficult to
debug if it fails (because you often won't be able to access an instance
on which cloud-init has failed to run). We have to be very
conservative.

> From my view it's a good opportunity to clean up some old irrelevant
> function parameters and simplify the whole code base.

As I said above, I don't disagree. :)

« Back to merge proposal