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

Revision history for this message
Eric Chen (eric-chen) 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.

This is a little tricky to me. There already a precedence design under /etc/grub.d/ folder.
If we want to put "console=ttyS0,115200 console=tty0" as a "nice to have" option, we should put it into 00xxxx. But now, we put it into 90sysconfig (almost the biggest number) and use this way to avoid taking precedence.

I don't know if other people have the same feeling. I suggest that we should find the stakeholders to discuss which one should have higher precedence and solve it in a correct way.

« Back to merge proposal