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

Revision history for this message
Dan Watkins (oddbloke) wrote :

(Checkpointing my review to respond to Ryan's comments; I've reached the top of parse_zfs.)

> The pattern is similar, there is some cross parsing helpers I'll need to sort out how that's shared, and while they all accept some form of probe_data, it does vary.

I think it's fine to keep shared helpers as module-level functions (unless they make more sense as methods to you, of course). If only shared helpers are in the module namespace then their presence there is meaningful.

> I'll look into classifying in the next iteration.

Thanks!

> I was initially interested in comparing the input yaml to a vmtest with what we get out of curtin block-discover after we boot into it; however some challenges include that our input isn't strictly ordered in the same way as block-discover, for example

I really like this idea. Given that block-discover is strictly ordered, we presumably _could_ get to a point where the vmtest input matches the output? I don't know a great deal about how the vmtests work, but if we had a flag that defaulted to False that indicated we should perform this test then we would be able to do this progressively.

(If we decide that integrating it in to the vmtests isn't worthwhile, a one-time pass capturing the probert output might make a solid starting point for unittests?)

« Back to merge proposal