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

Prep patches for modifying kargs #271

Merged
merged 3 commits into from
Jun 26, 2020
Merged

Prep patches for modifying kargs #271

merged 3 commits into from
Jun 26, 2020

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Jun 16, 2020

commit b7e7a01b6d308fe7b85751cefc1f31cd3793db17
Date:   Thu Jun 11 11:41:59 2020 -0400

    install: factor out function to walk BLS configs

    Prep for next patch.

commit 25c71d62f937d52970f85be2294b4f166983d908
Date:   Tue Jun 16 16:38:46 2020 -0400

    install: rename --firstboot-args to --append-firstboot-kargs

    Let's use a more precise name for this switch. First, specify that it
    *appends* to the list of firstboot arguments. Second, use `kargs`
    instead of just `args` to be more clear.

    This is prep for new switches which will use the same naming convention.

commit 70371c47ab76b78f522e4469906e05e1edd05c6d
Date:   Tue Jun 16 17:15:41 2020 -0400

    install: test platform ID restamping bits

    Factor out the closure and add some basic unit tests.

@jlebon jlebon marked this pull request as draft June 16, 2020 21:35
@jlebon jlebon marked this pull request as ready for review June 16, 2020 21:42
@jlebon
Copy link
Member Author

jlebon commented Jun 16, 2020

@lucab
Copy link
Contributor

lucab commented Jun 17, 2020

Code looks ok, I can stamp this later on if @bgilbert is fine with the new CLI flag arrangement.

@jlebon
Copy link
Member Author

jlebon commented Jun 17, 2020

Ahhh the rename to --append-firstboot-kargs shifts all the help strings to the right and we now go over 80 chars. And there doesn't seem to be a way to tell clap to put the help string on the next line and still keep the help string for all other options unindented.

I guess we can keep it as --firstboot-args, though I really wanted some consistency with --append-karg and --delete-karg.

@jlebon
Copy link
Member Author

jlebon commented Jun 17, 2020

Actually on that subject, couldn't we just "de-emphasize" (if not deprecate) and hide --firstboot-args now? The primary use case for this was network configs, and those are now much better covered by --copy-network, right?

@dustymabe
Copy link
Member

Actually on that subject, couldn't we just "de-emphasize" (if not deprecate) and hide --firstboot-args now? The primary use case for this was network configs, and those are now much better covered by --copy-network, right?

Maybe we could de-emphasize, probably not deprecate at least the underlying code path as --copy-network won't apply for the "automation via installer kargs" path.

@jlebon
Copy link
Member Author

jlebon commented Jun 19, 2020

OK, I updated this now to hide --firstboot-args!

jlebon added 3 commits June 25, 2020 13:02
Factor out the closure and add some basic unit tests.
We don't really want users to use this now. The primary use case was
configuring the network from the kernel cmdline. This has been obsoleted
by the nicer `--copy-network` approach.

We still need to keep supporting it for now though. It's used at least
by `coreos-installer.service` to auto-forward some kargs.
@jlebon jlebon merged commit e0a95ae into coreos:master Jun 26, 2020
@jlebon jlebon deleted the pr/persistent-args-prep branch June 26, 2020 13:09
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.

4 participants