Skip to content

feat: extend cmdline / metal provider for loading loading config from device#2018

Draft
tuunit wants to merge 1 commit intocoreos:mainfrom
tuunit:feat/extend-cmdline-options
Draft

feat: extend cmdline / metal provider for loading loading config from device#2018
tuunit wants to merge 1 commit intocoreos:mainfrom
tuunit:feat/extend-cmdline-options

Conversation

@tuunit
Copy link
Copy Markdown

@tuunit tuunit commented Feb 14, 2025

No description provided.

Copy link
Copy Markdown
Collaborator

@prestist prestist left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @tuunit! This is a great start and addresses a real need for bare metal config drive support.

I took a look through the implementation and it's mostly solid. A few things I noticed that could use some attention:

Critical Issue

There's a copy/paste bug around line 100:

case flagUserDataPath:
    if value == "" {
        logger.Info("user data path flag found but no value provided")
        continue
    }
    opts.DeviceLabel = value  // should be opts.UserDataPath = value

This would cause ignition.config.path to overwrite the device label instead of setting the path.

Other Feedback

Context cleanup: Missing a defer cancel() after creating the context on line 77. Could leak the context.

Silent failures: When the config file doesn't exist on the mounted device (lines 203-205), it returns nil, nil which gets parsed as empty config. Might be better to explicitly log this or return an error so users know what happened.

User feedback: The check on line 71 requires both parameters to be set, but if someone only provides one there's no clear error message telling them what's missing.

Dispatch pattern: The goroutine+dispatch wrapper (lines 79-96) seems more complex than needed for what's basically a single blocking operation with a timeout. Could probably simplify this.

What would help

  • Some test cases for the new functionality
  • Documentation for the new kernel parameters (ignition.config.device, ignition.config.path)
  • A PR description explaining the use case and how to use it

The core implementation looks good though - proper timeout handling, read-only mounts, cleanup with defer. Just needs these fixes and it should be ready to go.

Let me know if you want any help with the changes or have questions about the feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants