Fix podman pod create --infra-command and --infra-image #7621
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
Fixes #7167 |
libpod/pod.go
Outdated
|
|
||
| // InfraImage is the image a pod infra container will use to manage | ||
| // namespaces. | ||
| InfraImage string `json:"InfraImage,omitempty"` |
There was a problem hiding this comment.
Throw these in InfraContainerConfig
|
You've some lint unhappiness in here @rhatdan |
rhatdan
force-pushed
the
pods
branch
2 times, most recently
from
6119f5b to
91fd522
Compare
libpod/pod.go
Outdated
|
|
||
| // InfraImage is the image a pod infra container will use to manage | ||
| // namespaces. | ||
| InfraImage string `json:"infraImage,omitempty"` |
There was a problem hiding this comment.
Can you take these out of PodConfig now that they're in InfraContainerConfig?
There was a problem hiding this comment.
Light dawns on marble head. Fixed.
|
Still fighting test issues @rhatdan |
|
This is ready to go in, |
| @@ -104,6 +103,18 @@ func create(cmd *cobra.Command, args []string) error { | |||
| createOptions.Share = nil | |||
| } else { | |||
| createOptions.Share = strings.Split(share, ",") | |||
| if cmd.Flag("infra-command").Changed { | |||
| createOptions.InfraCommand, err = cmd.Flags().GetString("infra-command") | |||
There was a problem hiding this comment.
Why is that necessary? It seems rather exotic. A comment would be very helpful.
There was a problem hiding this comment.
Not sure it is exotic, since this type of code is all over the cli. Basically we want to make sure the user made a change using the CLI, rather then sending the default settings over to the server. That way the server can differentiate if the user wanted a change or just to take defaults.
There was a problem hiding this comment.
So we wouldn't take the defaults from the client containers.conf but from the server?
There was a problem hiding this comment.
What's the reasoning for that? Do we document that somewhere?
I think there's some inconsistency among the commands w.r.t. to which defaults come from the client and server.
I am not against the idea but just want to understand if we have some policy for that decision.
There was a problem hiding this comment.
All defaults should come from the server. Any defaults that come from the client are a bug. But I agree there are some coming from the client.
There was a problem hiding this comment.
@vrothberg I now have comments, PTAL
Currently infr-command and --infra-image commands are ignored from the user. This PR instruments them and adds tests for each combination. Signed-off-by: Daniel J Walsh <[email protected]>
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Currently infr-command and --infra-image commands are ignored
from the user. This PR instruments them and adds tests for
each combination.
Signed-off-by: Daniel J Walsh [email protected]