Code review comment for ~raharper/curtin:feature/block-discover

Instead of saying thanks on all the inline changes, I'll say it here:
thanks!

On Wed, May 01, 2019 at 06:03:45PM -0000, Ryan Harper wrote:
> > + if self.probe_data_key in probe_data:
> > + data = self.probe_data.get(self.probe_data_key)
>
> In some of the probe types, we'll have keys set but empty, for example
> if probert found no lvm data on the system, we see things like:
>
> storage:
> lvm: {}
>
> And the datatype parser handles the lack of data.
>
> That said, if probe data had something like:
>
> storage:
> lvm: None
>
> We would blow up on non-dict type, so maybe in the base class I can
> check for none-ish values and set the attribute to {}.

Ah, OK, I see. I think that would be an improvement but it probably
isn't vital.

> > + if not self.blockdev_data:
> > + raise ValueError('probe_data missing valid "blockdev" data')
> > +
> > + def parse(self):
> > + raise NotImplementedError()
>
> Probably. What does subclassing from ABC get us over the above?

If a subclass hasn't overriden an abstract method, we will get the error
on instantiation of the class. With the NotImplementedError route, we
only get it once we call the missing method.

I don't think it's hugely important provided we have good test coverage
so we will catch any such missing methods, perhaps just something to
consider for the next time we do something like this.

(It also gives static analysis tools a bit more to work on, so they can
also tell you via your editor when you've failed to correctly subclass
an ABC.)

> > +
> > + def blockdev_byid_to_devname(self, link):
> > + """ Lookup blockdev by devlink and convert to storage_config id. """
> > + bd_key = self.lookup_devname(link)
> > + if bd_key:
> > + return self.blockdev_to_id(self.blockdev_data[bd_key])
> > + return None
> > +
> > +
> > +class BcacheParser(ProbertParser):
> > +
> > + probe_data_key = 'bcache'
> > +
> > + def __init__(self, probe_data):
> > + super(BcacheParser, self).__init__(probe_data)
> > + self.backing = self.bcache_data.get('backing', {})
> > + self.caching = self.bcache_data.get('caching', {})
> > +
> > + def parse(self):
> > + """parse probert 'bcache' data format.
> > +
> > + Collects storage config type: bcache for valid
> > + data and returns tuple of lists, configs, errors.
> > + """
> > + configs = []
> > + errors = []
> > + for dev_uuid, bdata in self.backing.items():
> > + entry = self.asdict(dev_uuid, bdata)
> > + if entry:
> > + try:
> > + validate_config(entry)
> > + except ValueError as e:
> > + errors.append(e)
> > + continue
> > + configs.append(entry)
> > + return (configs, errors)
> > +
> > + def asdict(self, backing_uuid, backing_data):
> > + """ process a specific bcache entry and return
> > + a curtin storage config dictionary. """
> > +
> > + def _sb_get(data, attr):
>
> I see what you're saying (TIL closures vs. anonymous functions). If
> feels a bit strange to me to do that.

Fair enough!