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

On Tue, Mar 26, 2019 at 03:27:24PM -0000, Gonéri Le Bouder 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. :)
>
> I will be happy to refresh my patch to take that into account.

Thanks!

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

Excellent, I look forward to it!

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

Aha, OK, I see what you mean. For clarity, mount_cb doesn't use
cloud-init's definition of 'distro' at all; it's calling
platform.system() which is in the Python stdlib[0].

[0] https://docs.python.org/3/library/platform.html#platform.system

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

I think one patch dropping both rw and sync would be acceptable.