cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

Diff comments:

> diff --git a/cloudinit/sources/__init__.py b/cloudinit/sources/__init__.py
> index 9a43fbe..ec0fe2a 100644
> --- a/cloudinit/sources/__init__.py
> +++ b/cloudinit/sources/__init__.py
> @@ -78,6 +103,52 @@ class DataSource(object):
>      def __str__(self):
>          return type_utils.obj_name(self)
>  
> +    def _get_standardized_metadata(self):
> +        """Return a dictionary of standardized metadata keys."""
> +        return {'v1': {
> +            'public-hostname': self.get_hostname(),
> +            'public-ipv4-address': self.metadata.get('public-ipv4'),
> +            'public-ipv6-address': self.metadata.get('ipv6-address'),
> +            'instance-id': self.get_instance_id(),
> +            'cloud-name': self.cloud_name,
> +            'region': self.region,
> +            'availability-zone': self.metadata.get('availability_zone')}}
> +
> +    def get_data(self):
> +        """Datasources implement _get_data to setup metadata and userdata_raw.
> +
> +        Minimally, the datasource should return a boolean True on success.
> +        """
> +        return_value = self._get_data()
> +        json_file = os.path.join(self.paths.run_dir, INSTANCE_JSON_FILE)

you can out-dent everything below here if you do:
  if not return_value:
      return return_value

  other_stuff_here.


I generally like that "get out early" flow.
I know sometimes others do not.

> +        if return_value:
> +            instance_data = {
> +                'ds': {
> +                    'meta-data': self.metadata,
> +                    'user-data': self.get_userdata_raw(),
> +                    'vendor-data': self.get_vendordata_raw()}}
> +            instance_data.update(
> +                self._get_standardized_metadata())
> +            try:
> +                # Process content base64encoding unserializable values
> +                content = util.json_dumps(instance_data)
> +                # Strip base64: prefix and return base64-encoded-keys
> +                processed_data = process_base64_metadata(json.loads(content))
> +                content = util.json_dumps(processed_data)
> +            except TypeError as e:
> +                LOG.warning('Error persisting instance-data.json: %s', str(e))

Just blow up on this stuff?
How could we get here ?

Also, your error is not exactly correct, you're not "persisting" it in any of the above.  You do that below, and dont' handle the error.  (at least in my head "persisting" is "writing to filesystem").

> +                return return_value
> +            except UnicodeDecodeError as e:
> +                LOG.warning('Error persisting instance-data.json: %s', str(e))
> +                return return_value
> +            util.write_file(json_file, content, mode=0o600)
> +        return return_value
> +
> +    def _get_data(self):
> +        raise NotImplementedError(
> +            'Subclasses of DataSource must implement _get_data which'
> +            ' sets self.metadata, vendordata_raw and userdata_raw.')
> +
>      def get_userdata(self, apply_filter=False):
>          if self.userdata is None:
>              self.userdata = self.ud_proc.process(self.get_userdata_raw())
> @@ -91,6 +162,34 @@ class DataSource(object):
>          return self.vendordata
>  
>      @property
> +    def cloud_name(self):
> +        """Return lowercase cloud name as determined by the datasource.
> +
> +        Datasource can determine or define its own cloud product name in
> +        metadata.
> +        """
> +        if self._cloud_name:
> +            return self._cloud_name
> +        if self.metadata and self.metadata.get(METADATA_CLOUD_NAME_KEY):
> +            cloud_name = self.metadata.get(METADATA_CLOUD_NAME_KEY)
> +            if isinstance(cloud_name, six.string_types):
> +                self._cloud_name = cloud_name.lower()
> +            LOG.debug(

this feels like a warning to me. something is wrong here if we have non-string keys in metadata.
no?

> +                'Ignoring metadata provided key %s: non-string type %s',
> +                METADATA_CLOUD_NAME_KEY, type(cloud_name))
> +        else:
> +            self._cloud_name = self._get_cloud_name().lower()
> +        return self._cloud_name
> +
> +    def _get_cloud_name(self):
> +        """Return the datasource name as it frequently matches cloud name.
> +
> +        Should be overridden in subclasses which can run on multiple
> +        cloud names, such as DatasourceEc2.
> +        """
> +        return self.dsname
> +
> +    @property
>      def launch_index(self):
>          if not self.metadata:
>              return None


-- 
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/330115
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:unify-datasource-get-data into cloud-init:master.