Feat: Support Running Sidecar with a Command. #2449
Conversation
This PR addresses issue zalando#2448 . Some containers may not have entry points, if this is the case they would need to be run using a command. This change extends the definition of sidecar so that there is an optional command field. If the field is present then the container will be run using that command. This is a two line change that is fully backward compatible.
tabbyl21
requested review from
sdudoladov,
Jan-M,
FxKu,
jopadi,
idanovinda and
hughcapet
as code owners
|
Hi @Jan-M , @sdudoladov , @FxKu , @idanovinda , @hughcapet, @jopadi Could you please review this PR? |
|
Hi @FxKu could you please review this PR? It is similar to a PR you recently reviewed. Thanks |
tabbyl21
changed the title
tabbyl21
changed the title
|
Any updates? |
| @@ -220,6 +220,7 @@ type Sidecar struct { | |||
| DockerImage string `json:"image,omitempty"` | |||
| Ports []v1.ContainerPort `json:"ports,omitempty"` | |||
| Env []v1.EnvVar `json:"env,omitempty"` | |||
| Command []string `json:"command,omitempty" protobuf:"bytes,3,rep,name=command"` | |||
There was a problem hiding this comment.
why is protobuf:"bytes,3,rep,name=command" necessary? We have it nowhere else. Is it similar to filter inputs using pattern in the CRD schema?
There was a problem hiding this comment.
apologies just seeing this comment. I can remove that part
|
@tabbyl21 sorry for not reacting for a looong time. We are usually a bit reluctant to changes that offer more config mean and adding complexity. After taking a closer look I get now why this change is needed. Being able to run any command via sidecar just sounded too scary to me first (well, maybe because it is). If you could answer my one question we could merge it then. Try to run codegen (./hack/update-codegen.sh) to see if something changes. |
|
One question here. |
It is a []string so it includes the args, too |
|
@FxKu apologies didn't see your repsonses! I have addressed your comment! Let me know what changes need to be made to merge this in! |
|
Thanks @tabbyl21 for coming back. Can you check if the new field can be covered by a unit test? I'm not sure how much coverage we have there for sidecars. And it would be nice if the new command possibility can be included in our sidecar documentation. |
|
👍 |
|
👍 |
This PR addresses issue #2448 . Some containers may not have entry points, if this is the case they would need to be run using a command. This change extends the definition of sidecar so that there is an optional command field. If the field is present then the container will be run using that command. This is a two line change that is fully backward compatible.