dannf / ubuntu-server-netboot

Other
43 stars 9 forks source link

Refactor bootloader configuration file changes #15

Closed dannf closed 3 years ago

dannf commented 3 years ago

We basically need to make the same changes to both the pxelinux and GRUB configuration files - i.e., adding kernel parameters. Today we do this using two copies of mostly identical code. Recent changes that required kernel parameter manipulation have shown that it is easy to update one and forget to update the other. This change aims to consolidate that code to prevent that.

We centralize all of the kernel parameter logic in the new function setup_kernel_params() that can operate on any subclass of BootloaderConfig. We provide such subclasses for GRUB and pxelinux, which have the internal knowledge for creating and editing these files (which isn't much). Since setup_kernel_params() is called for both files, any kernel parameter logic changes should get applied to both equivalently.

Types of changes

Description

Checklist:

Steps to Test This Pull Request

Expected behavior

Related Issue

Additional context

dannf commented 3 years ago

Yeah it is so nice to consolidate grub and pxelinux. I am +1 on this pull request.

Additionally, two more information items for your reference:

  1. docstring style is suggested to be consistent (single or double quote)

Thanks! Normally I'd fix the docstring inconsistency you pointed out, but I noticed that we're starting with inconsistent quote styles, so being consistent in this commit would still leave the overall project inconsistent :(. I'll go ahead and merge as-is, and look forward to a black + pre-commit hook to fix it up. Speaking of which, were you planning on introducing black in a PR or should I?

tai271828 commented 3 years ago

In this case, going ahead and merging as-is is quite making sense to me.

I will introduce black + pre-commit later on.

Yeah it is so nice to consolidate grub and pxelinux. I am +1 on this pull request. Additionally, two more information items for your reference:

  1. docstring style is suggested to be consistent (single or double quote)

Thanks! Normally I'd fix the docstring inconsistency you pointed out, but I noticed that we're starting with inconsistent quote styles, so being consistent in this commit would still leave the overall project inconsistent :(. I'll go ahead and merge as-is, and look forward to a black + pre-commit hook to fix it up. Speaking of which, were you planning on introducing black in a PR or should I?