Don't rename pod if container has the same name #12726
Conversation
hikhvar
force-pushed
the
remove-superflous-pod-rename
branch
from
120e2f1 to
4dfe511
Compare
pkg/domain/infra/abi/play.go
Outdated
| @@ -15,6 +15,13 @@ import ( | |||
| "github.com/containers/common/libimage" | |||
| "github.com/containers/common/pkg/config" | |||
| "github.com/containers/image/v5/types" | |||
| "github.com/ghodss/yaml" | |||
There was a problem hiding this comment.
I think that is a left over of my IDE. It has auto sorting of imports enabled. The sorting is configured for our internal projects.
There was a problem hiding this comment.
@hikhvar Could you please amend and remove this.
|
@mheon PTAL |
|
@hikhvar Newly added tests are failing. |
hikhvar
force-pushed
the
remove-superflous-pod-rename
branch
from
4dfe511 to
31591af
Compare
|
Fixes the tests, reverted the import reordering. PTAL. |
|
Hm. Still not sure why this changed originally to prefixing container names (you've identified the commit but it doesn't list logic), but since we are and have been for some time, no reason not to do this. LGTM. |
|
/lgtm |
hikhvar
force-pushed
the
remove-superflous-pod-rename
branch
from
31591af to
bd6bcf7
Compare
|
Had a look over the code. Saw I removed a check too much. I added an integration test to prevent this error in the future. Sorry for the inconvenience. PTAL. |
|
Updates LGTM |
|
rebase and repush. |
We enforce the naming scheme "<podname>-<containername>" here [1]. Therefore we must not rename the pod in case of a naming conflict between pod name and container name. Not renaming the pod increases the usability for the user and easies scripting based on the name. Otherwise a user must set some label to reliable find a pod after creation. Or have to implement the renaming logic in the script. [1] https://github.com/containers/podman/blob/main/pkg/specgen/generate/kube/kube.go#L140 Fixes containers#12722 Signed-off-by: Christoph Petrausch <[email protected]>
hikhvar
force-pushed
the
remove-superflous-pod-rename
branch
from
bd6bcf7 to
4191616
Compare
|
Rebased and pushed again. |
| fmt.Sprintf("a container exists with the same name (%q) as the pod in your YAML file; changing pod name to %s_pod\n", podName, podName)) | ||
| podName = fmt.Sprintf("%s_pod", podName) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm just wondering if it makes sense to keep this warning in play, although maybe changing the wording a bit, to let the user know of the collision so they could adjust afterwards if they wanted to. I agree with dropping the auto renam though.
There was a problem hiding this comment.
The warning is missleasing. There never will be a container with the same name as the pod, since all container names in the pod will be prefixed with the pod name.
|
One last test to convince, one question, and I've restarted the unhappy test. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hikhvar, 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
bot
added
the
approved
github-actions
bot
added
the
locked - please file new issue/PR
We enforce the naming scheme "podname-containername" here [1].
Therefore we must not rename the pod in case of a naming conflict
between pod name and container name. Not renaming the pod increases the
usability for the user and easies scripting based on the name. Otherwise
a user must set some label to reliable find a pod after creation. Or
have to implement the renaming logic in the script.
[1] https://github.com/containers/podman/blob/main/pkg/specgen/generate/kube/kube.go#L140
Fixes #12722
Signed-off-by: Christoph Petrausch [email protected]