-
Notifications
You must be signed in to change notification settings - Fork 113
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
add (missing) deploy.resources.devices.options #691
Conversation
@@ -27,6 +27,7 @@ type DeviceRequest struct { | |||
Driver string `yaml:"driver,omitempty" json:"driver,omitempty"` | |||
Count DeviceCount `yaml:"count,omitempty" json:"count,omitempty"` | |||
IDs []string `yaml:"device_ids,omitempty" json:"device_ids,omitempty"` | |||
Options Mapping `yaml:"options,omitempty" json:"options,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not doing anything with this yet. Should we wait for the upcoming work to add this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are not because this was missing in compose-go, but moby API has support for options (sorry, can't post a direct link to DeviceRequest nested struct)
Driver-specific options, specified as a key/value pairs. These options are passed directly to the driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, re-reading my comment I realise I did not express myself correctly. I meant, in compose-go you defined the Options
but not doing any actual implementation with it. All I am saying is should we save this change for later for when we augment the options section or should we already implement the moby api call in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compose-go never includes implementation; this is docker/compose role to do so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this PR merged, we need to handle manually the parsing of options
attributes, I think this is what @jhrotko wanted to mention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, +added a test case to cover loading DeviceRequest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a464071
to
fe8dfec
Compare
types/device.go
Outdated
@@ -92,6 +93,13 @@ func (d *DeviceRequest) DecodeMapstructure(value interface{}) error { | |||
d.IDs = ids | |||
d.Count = DeviceCount(len(ids)) | |||
} | |||
|
|||
if options, ok := v["options"].(map[string]any); ok { | |||
d.Options = Mapping{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we do this d.Options = Mapping{}
even if there isn't any option defined to avoid nil issue on the client side?
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
fe8dfec
to
d01d1c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
defined by the spec but missing in go struct for some legacy reasons