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-resources CLI typo #35970

Merged
merged 1 commit into from Jan 10, 2018

Conversation

@RenaudWasTaken
Copy link
Contributor

commented Jan 9, 2018

Signed-off-by: Renaud Gaubert rgaubert@nvidia.com

- What I did
The CLI argument node-generic-resource was incorrectly spelled.
That create a bug where it was not merged into the config when specified in the a config file.

/ping @thaJeztah
Do you think we can get this in 18.01 ?

Thanks!

Fix node-generic-resources CLI typo
Signed-off-by: Renaud Gaubert <rgaubert@nvidia.com>
@AkihiroSuda
Copy link
Member

left a comment

LGTM

This was regression in ebe1431

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 10, 2018

Do you think we can get this in 18.01 ?

Probably won't make it for 18.01, but I can open an internal request to suggest back porting it to 17.12 if there will be a patch release for that.

@thaJeztah
Copy link
Member

left a comment

LGTM

@thaJeztah thaJeztah merged commit 178d027 into moby:master Jan 10, 2018

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 38684 has succeeded
Details
janky Jenkins build Docker-PRs 47431 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7819 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18923 has succeeded
Details
z Jenkins build Docker-PRs-s390x 7777 has succeeded
Details
@RenaudWasTaken

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2018

Thanks!

@3XX0

This comment has been minimized.

Copy link

commented Jan 10, 2018

@thaJeztah Back porting it to 17.12.1 if any, would be a good idea.

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 10, 2018

Yes, I opened a ticket in our "release tracking" issue tracker (that's a mouthful); but this change on itself likely won't warrant a patch release (only P0 issues or enough P1 issues usually are reason to do a patch release).

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

Looking at this change again, and now wondering if this change was correct; IIRC, there's code somewhere that translates singular (flag) to plural (config file), if an option can be specified multiple times.

Looking into this with @vdemeester to verify

@@ -67,7 +67,7 @@ func installCommonConfigFlags(conf *config.Config, flags *pflag.FlagSet) {

flags.StringVar(&conf.MetricsAddress, "metrics-addr", "", "Set default address and port to serve the metrics api on")

flags.Var(opts.NewListOptsRef(&conf.NodeGenericResources, opts.ValidateSingleGenericResource), "node-generic-resource", "Advertise user-defined resource")
flags.Var(opts.NewListOptsRef(&conf.NodeGenericResources, opts.ValidateSingleGenericResource), "node-generic-resources", "Advertise user-defined resource")

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jan 26, 2018

Member

This should be

opts.NewNamedListOptsRef("node-generic-resources", &conf.NodeGenericResources, opts.ValidateSingleGenericResource)
@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

Opened #36125 to address this issue

@RenaudWasTaken RenaudWasTaken deleted the RenaudWasTaken:cli-fix branch Mar 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.