Code review comment for ~jawn-smith/ubuntu/+source/ubuntu-release-upgrader:ubuntu/jammy

Revision history for this message
Dave Jones (waveform) wrote :

I'm afraid there's a few issues:

* I'd much rather parse the YAML with a yaml library given there's so many possible representations here (which cloud-init is free to use when it converts network-config to the netplan config). For example, match: {driver: "bcmgenet smsc95xx lan78xx"} is valid YAML. I know adding dependencies is generally frowned upon, but python3-yaml is seeded on the pi server and desktop images (presumably because cloud-init and ubuntu-advantage-tools both depend on it, among many other things) so adding it as a dep of u-r-u won't *actually* pull in anything new, would simplify this code, and deal with all possible variants of the configuration.

* If you remove the match/driver: clause, the set-name: clause becomes invalid (it requires a match: clause). In this case it's probably sufficient to just remove the set-name: clause along with the match: clause.

* Ideally the config_txt variable in the various tests should be renamed; it's not the config.txt on the boot partition that's being modified (unlike the prior tests concerned with removing u-boot), it's the netplan yaml.

Other than that: nice to see a good set of tests, covering all the netplan config scenarios (match present, not present, present but only in certain files)!

review: Needs Fixing

« Back to merge proposal