Code review comment for ~dashmage/charm-sysconfig:bug-2012581/inherit-existing-kernel-params

Revision history for this message
Ashley James (dashmage) wrote :

> It's LGTM for implementation part, but I think the unit test for it is
> required.

There seems to be unit tests already for the `update_grub_file` function [1]. Would I need to add something additionally to test for the change made in the template file?

[1]: https://git.launchpad.net/charm-sysconfig/tree/src/tests/unit/test_lib.py#n250

> Also please provide more detail to explain why this change works.

```
-{% else -%}
-GRUB_CMDLINE_LINUX_DEFAULT=""
```

This is to allow inheriting the pre-existing kernel cmdline parameters the default option. With the charm no longer resetting the value of `GRUB_CMDLINE_LINUX_DEFAULT` to an empty string, it'll only append to the already existing values (ex: coming from MAAS).

```
-GRUB_CMDLINE_LINUX_DEFAULT="$GRUB_CMDLINE_LINUX_DEFAULT console=ttyS0,115200 console=tty0"
+GRUB_CMDLINE_LINUX_DEFAULT="console=ttyS0,115200 console=tty0 $GRUB_CMDLINE_LINUX_DEFAULT"
```

These parameters have been switched around so that the pre-existing console values (ex: coming from MAAS) will not be overwritten by the charm. According to my understanding, the console parameters (if defined) from `$GRUB_CMDLINE_LINUX_DEFAULT` will take precedence since they are declared the last.

« Back to merge proposal