Fix Docker API DeviceMapping for CDI devices#28495
Fix Docker API DeviceMapping for CDI devices#28495Honny1 wants to merge 1 commit intocontainers:mainfrom
Conversation
4c035d2 to
202ce2c
Compare
Luap99
left a comment
There was a problem hiding this comment.
an e2e compose test like test/compose/cdi_device/docker-compose.yml would make sense, I guess just add a second device to the config and then pass it as normal device in compose?
| } | ||
| for _, r := range cc.HostConfig.Resources.DeviceRequests { | ||
| if r.Driver == "cdi" { | ||
| if strings.EqualFold(r.Driver, "cdi") { |
There was a problem hiding this comment.
do you have any references why this should support upper case?
https://github.com/docker/cli/blob/950401cf07cf590e921d94ace7311977dd582a3c/cli/command/container/opts.go#L606-L609
https://github.com/docker/compose/blob/d518da24193f88d9ecf8e847fe2f89b2344b6405/pkg/compose/create.go#L684-L687
suggest they send the string lowercase
There was a problem hiding this comment.
My original plan was to take a defensive approach, but I realize now that it doesn't make sense. I will fix it.
I'm not sure about this. Newer versions (since docker/compose#12184) of Compose send CDI devices through |
202ce2c to
73775ce
Compare
|
I am not sure if this change fixes the reported issues, as I cannot directly verify them. The main goal of this change is to make defensive improvements for edge cases that might occur with older versions of Compose or other clients. |
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
73775ce to
f374f2c
Compare
|
It seems like a reasonable change regardless. I wonder if the people seeing this are all on Compose v1? We dropped all our tests for that... |
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?