Code review comment for ~redriver/cloud-init:frbsd-azure-branch

> > Thanks for you careful review and valuable comments.
> >
> > > Overall, comments, much of the code needs some unit tests. We simply
> can't
> > > avoid breaking things inadvertently without them
> > get_resource_disk_on_freebsd
> > > and the networking code specifically.
> > All right. I'll add some unit tests.
>
> Great.
>
> >
> > > Please bear with me... you have a lot of good things here, we just need
> > > to work out some details.
> > >
> > > - adding the specific config/cloud.cfg-freebsd-azure
> > > I'd much prefer we adjust the already freebsd specific cloud.cfg-
> freebsd.
> > >
> > > The changes you have there are:
> > > - add Azure to the datasource_list. I think its right to just put
> > > Azure before Openstack and Ec2 so we have the list of 4 sources.
> > Ok.
> > > - you disabled growpart and resizefs
> > > I wonder why these were not disabled by the original freebsd work.
> > > Can you explain that? If they just aren't going to work, then lets
> > > remove them from the list and update the docstring to mention lack
> > > of support on freebsd. If they are only broken on Azure, lets figure
> > > out why.
> > On Azure, FreeBSD image used a fixed size VHD, so gpart is not necessary.
> > Anyway, there is some error reported in the log if I enabled it. For the
> same
> > reason, I disabled resizefs on Azure. Well, if we decide to use the same
> > cloud-init.cfg for FreeBSD. I have to fix those error message and enable
> > growpart and resizefs.
>
> OK, Hopefully we could use Jake's suggestion above and get resizefs and
> growpart working even if they aren't needed on azure. They're in general good
> things.
>
> > > - default user changes
> > > - removing of the default 'name'. This should not be necessary as
> > > linux uses the datasource provided username on azure when its
> there.
> > > - changing of shell to csh: my understanding (per git log) is that
> tcsh
> > > is the default shell on freebsd, so using that makes more sense.
> > > if Azure wishes to carry a different default shell, they can do
> that
> > > in local config. Generally though, commonality is better.
> > > - removal of lock_passwd: azure can carry that locally if they want.
> > For unknown reason, if I did not remove the default 'name' on Azure, it will
> > stop the data source provided username. Ok, that may be a bug on Azure. In
> > addition, the default username 'freebsd' is created but locked passwd, then
> > why do we need it?
>
> Maybe i wasn't clear. I was just saying that this all works as expected on
> Ubuntu (linux). We have a default config with a user 'ubuntu', but on azure,
> where a user is provided via the datasource, it overrides just that. So this
> should be able to work correctly.
>
Ok. I see. Anyway, I observed it does not work as expected with default user.
I'll investigate it.

> > > - why the changes set_passwd?
> > > You made 2 changes here:
> > > a.) use -H 0 and -h 0 rather than just -H or -h
> > > I'm fine to be explicit, but it seems like it shouldnt be
> necessary
> > > or it would never have worked.
> > > b.) you use a shell to 'echo ... |' when we were previously just
> > > providing the password as standard in to the command (via subp
> > > 'data').
> > > the shell is more processes to do the same work, did it not work
> for
> > > you before?
> > It does not work because 'pw' command only accepts password from stdin. I do
> > not why the origin one works, I guess maybe it works on early FreeBSD
> version.
> > Anyway, we should fix it.
>
> Sorry, again, I wasn't clear.
> Essentially your changes do this:
> util.subp("echo password | pw usermod USER -H 0", shell=True)
> That should be pretty much the same thing as:
> util.subp(['pw', 'usermod', USER, '-H', '0'], data="password")
>
Ok, that looks better. I'll apply it.

> And the second format there does not invoke shell, but puts the bytes
> 'password' into the stdin of the command.
>
> > > comments on generate_fallback_config:
> > > - you have lots of LOG.info() that is clearly developer debug like
> stuff
> > > drop what you can to debug.
> > > - more generically, do you think that parsing 'ifconfig' is the right
> > > way to do this for freebsd?
> > I'll remove those LOG.info(). If we have to use builtin command, for
> example,
> > 'ifconfig', we have to parse 'ifconfig' output. Maybe there are some other
> > utility commands can help, but it takes more time to investigate. I think we
> > can polish this if I find better alternatives.
>
> I'm not opposed to using 'ifconfig', just didn't know if that was "the right
> thing" to use on that platform. Ie, if I was doing new linux support, i'd use
> the 'ip' command rather than 'ifconfig'.
Unfortunately, there is no counterpart for 'ip' on FreeBSD. That is why I have to parse ifconfig.