cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Thanks for this branch Gonéri. Minor nits from me, but otherwise looks good.

Please reset 'Needs review' status when you get a chance to address or resolve comments.


Diff comments:

> diff --git a/cloudinit/sources/DataSourceNoCloud.py b/cloudinit/sources/DataSourceNoCloud.py
> index fcf5d58..9912c87 100644
> --- a/cloudinit/sources/DataSourceNoCloud.py
> +++ b/cloudinit/sources/DataSourceNoCloud.py
> @@ -35,6 +35,27 @@ class DataSourceNoCloud(sources.DataSource):
>          root = sources.DataSource.__str__(self)
>          return "%s [seed=%s][dsmode=%s]" % (root, self.seed, self.dsmode)
>  
> +    def _get_devices(self, label):
> +        devlist = []
> +        if util.is_FreeBSD():
> +            for p in ['/dev/msdosfs/' + label, '/dev/iso9660/' + label]:

Since you are setting devlist in the else clause, you could drop the devlist = [] above and
        if util.is_FreeBSD():
            devlist = [
                p for p in ['/dev/msdosfs/' + label, '/dev/iso9660/' + label]
                if os.path.exists(p)]

> +                if os.path.exists(p):
> +                    devlist.append(p)
> +        else:
> +            # Query optical drive to get it in blkid cache for 2.6 kernels
> +            util.find_devs_with(path="/dev/sr0")
> +            util.find_devs_with(path="/dev/sr1")
> +
> +            fslist = util.find_devs_with("TYPE=vfat")
> +            fslist.extend(util.find_devs_with("TYPE=iso9660"))
> +
> +            label_list = util.find_devs_with("LABEL=%s" % label.upper())
> +            label_list.extend(util.find_devs_with("LABEL=%s" % label.lower()))
> +
> +            devlist = list(set(fslist) & set(label_list))
> +            devlist.sort(reverse=True)
> +        return devlist
> +
>      def _get_data(self):
>          defaults = {
>              "instance-id": "nocloud",
> diff --git a/config/cloud.cfg.tmpl b/config/cloud.cfg.tmpl
> index 25db43e..d3c50dd 100644
> --- a/config/cloud.cfg.tmpl
> +++ b/config/cloud.cfg.tmpl
> @@ -33,7 +33,7 @@ preserve_hostname: false
>  {% if variant in ["freebsd"] %}
>  # This should not be required, but leave it in place until the real cause of
>  # not beeing able to find -any- datasources is resolved.
> -datasource_list: ['ConfigDrive', 'Azure', 'OpenStack', 'Ec2']
> +datasource_list: ['ConfigDrive', 'Azure', 'OpenStack', 'Ec2', 'NoCloud']

I don't think this should be required, because of util.get_builtin_config returning a full list of datasources. But if it is required on some system you have seen, then I think you should order NoCloud first, before ConfigDrive. Then FreeBSD would align with our default settings in cloudinit/settings.py.

Also since you are touching this file anyway, can you replace "beeing able to find -any-' with "finding -any-".

>  {% endif %}
>  # Example datasource config
>  # datasource:


-- 
https://code.launchpad.net/~goneri/cloud-init/+git/cloud-init/+merge/365630
Your team cloud-init commiters is requested to review the proposed merge of ~goneri/cloud-init:freebsd_nocloud into cloud-init:master.