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

Fix "--node-generic-resource" singular/plural #36125

Merged

Conversation

thaJeztah
Copy link
Member

Daemon flags that can be specified multiple times use singlar names for flags, but plural names for the configuration file.

To make the daemon configuration know how to correlate the flag with the corresponding configuration option, opt.NewNamedListOptsRef() should be used instead of opt.NewListOptsRef().

Commit 6702ac5 (#35970) attempted to fix the daemon not corresponding the flag with the configuration file option, but did so by changing the name of the flag to plural.

This patch reverts that change, and uses opt.NewNamedListOptsRef() instead.

ping @vdemeester @AkihiroSuda @RenaudWasTaken

Daemon flags that can be specified multiple times use
singlar names for flags, but plural names for the configuration
file.

To make the daemon configuration know how to correlate
the flag with the corresponding configuration option,
`opt.NewNamedListOptsRef()` should be used instead of
`opt.NewListOptsRef()`.

Commit 6702ac5 attempted
to fix the daemon not corresponding the flag with the configuration
file option, but did so by changing the name of the flag
to plural.

This patch reverts that change, and uses `opt.NewNamedListOptsRef()`
instead.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐸

@RenaudWasTaken
Copy link
Contributor

Thanks for looking into this @thaJeztah !

@thaJeztah
Copy link
Member Author

ping @AkihiroSuda @dnephin PTAL

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

6 participants