Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSModifier: Add services, modules, and kernel cmdline in EMU API #11080

Open
wants to merge 3 commits into
base: 3.0-dev
Choose a base branch
from

Conversation

elainezhao96
Copy link
Contributor

@elainezhao96 elainezhao96 commented Nov 14, 2024

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

  • Ready to merge

Summary

What does the PR accomplish, why was it needed?
Add services, modules, and kernel cmdline in EMU API
related PR that updates trident: https://dev.azure.com/mariner-org/ECF/_git/trident/pullrequest/20937

Change Log
  • update extraCommandLine to a list of strings
  • Add services, modules, and kernel cmdline in EMU API
Does this affect the toolchain?

NO

Test Methodology
  • Will have trident-e2e pipeline verify the change

@elainezhao96 elainezhao96 marked this pull request as ready for review November 15, 2024 23:35
@elainezhao96 elainezhao96 requested review from a team as code owners November 15, 2024 23:35
@@ -7,14 +7,15 @@ import (
)

// KernelExtraArguments defines one or more extra kernel arguments.
type KernelExtraArguments string
type KernelExtraArguments []string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to do the 'kernel command-line as a list' change in a separate PR.

if err != nil {
return err
for _, arg := range e {
err := validateKernelArgumentsFormat(arg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of the making the kernel command-line a list is so that MIC will handle the character escaping/quoting on behalf of the user. Hence, the validateKernelArgumentsFormat call should no longer be required.

This should also allow you to remove the KernelExtraArguments type as well.

return err
}

moduleMap := make(map[string]int)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wanted to avoid duplicate code, you could create a ModuleList type under imagecustomizerapi and move this validation logic under that.

if err != nil {
return err
for _, arg := range kernelExtraArguments {
err = bootCustomizer.AddKernelCommandLine(arg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BootCustomizer.AddKernelCommandLine function should be changed to take a list.

Also, use GrubArgsToString to convert the args list to escaped/quoted string.

@@ -192,7 +193,7 @@ func kernelCommandLineToImager(kernelCommandLine imagecustomizerapi.KernelComman
}

imagerKernelCommandLine := configuration.KernelCommandLine{
ExtraCommandLine: string(kernelCommandLine.ExtraCommandLine),
ExtraCommandLine: strings.Join(kernelCommandLine.ExtraCommandLine, " "),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the GrubArgsToString function to convert the list to a escaped/quoted string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants