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

Dan Watkins <email address hidden> writes:

> 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. :)

I will be happy to refresh my patch to take that into account.

>> 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. :)

For the record, I've also started that:
https://github.com/goneri/cloud-init/commit/5129be3bdc947c629c971c08c811db1997454edf

It's a network renderer for FreeBSD based on what already exists. It's
not ready yet for review. I will push a PR later.

>> 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.)

Yes, I put the quotes on purpose. I was refering to the
./cloudinit/distros/freebsd.py file which is a directory called
'distro'.

>> 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. :)

Thanks for the clarification. Can I push a patch for review without the
'rw' and 'sync'? Or should I instead refact the current patch and keep
the sync=True by default.

Cheers,
--
    Gonéri Le Bouder