Skip to content
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

Allow '/' to prefix container names to match Docker #16820

Merged
merged 1 commit into from
Jan 7, 2023

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Dec 12, 2022

Fixes: #16663

Signed-off-by: Daniel J Walsh [email protected]

Does this PR introduce a user-facing change?

Podman now supports container names beginning with a '/' to match Docker behaviour.

@openshift-ci openshift-ci bot added do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Dec 12, 2022
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's more to unpack since Docker strips off the leading '/'. '/foo' create the name "foo" and it will also be stripped of in regexes. Yet ^/foo$ is not a valid regex to me.

@Luap99
Copy link
Member

Luap99 commented Dec 13, 2022

I think there's more to unpack since Docker strips off the leading '/'. '/foo' create the name "foo" and it will also be stripped of in regexes. Yet ^/foo$ is not a valid regex to me.

docker always stores the name with a leading /, if you look at the API response they always include the slash. see the compat API for example

Name: fmt.Sprintf("/%s", l.Name()),

So if you apply a filter via docker api we must also add the slash to the name before running the filter regex.

@rhatdan rhatdan force-pushed the names branch 3 times, most recently from 58572cd to cb9eb71 Compare December 14, 2022 21:18
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2022
@@ -139,3 +140,11 @@ func IsCheckpointImage(ctx context.Context, namesOrIDs []string) (bool, error) {
}
return true, nil
}

func RemoveSlash(input []string) []string {
var output []string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can initialize this with make([]string, 0, len(input)) to ensure we only do one allocation

@rhatdan
Copy link
Member Author

rhatdan commented Dec 23, 2022

@vrothberg @Luap99 @mheon PTAL

@mheon
Copy link
Member

mheon commented Dec 23, 2022

Code LGTM

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 25, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 26, 2022
@rhatdan rhatdan added the 4.4 label Jan 7, 2023
@rhatdan
Copy link
Member Author

rhatdan commented Jan 7, 2023

@containers/podman-maintainers PTAL

@rhatdan
Copy link
Member Author

rhatdan commented Jan 7, 2023

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhatdan rhatdan added the lgtm Indicates that a PR is ready to be merged. label Jan 7, 2023
@openshift-merge-robot openshift-merge-robot merged commit 5de8cd7 into containers:main Jan 7, 2023
@baude
Copy link
Member

baude commented Feb 3, 2023

/cherry-pick v4.4

@openshift-cherrypick-robot
Copy link
Collaborator

@baude: #16820 failed to apply on top of branch "v4.4":

Applying: Allow '/' to prefix container names to match Docker
Using index info to reconstruct a base tree...
M	cmd/podman/containers/attach.go
M	cmd/podman/containers/checkpoint.go
M	cmd/podman/containers/commit.go
M	cmd/podman/containers/exec.go
M	cmd/podman/containers/exists.go
M	cmd/podman/containers/export.go
M	cmd/podman/containers/init.go
M	cmd/podman/containers/kill.go
M	cmd/podman/containers/logs.go
M	cmd/podman/containers/mount.go
M	cmd/podman/containers/pause.go
M	cmd/podman/containers/port.go
M	cmd/podman/containers/prune.go
M	cmd/podman/containers/rename.go
M	cmd/podman/containers/restart.go
M	cmd/podman/containers/restore.go
M	cmd/podman/containers/rm.go
M	cmd/podman/containers/runlabel.go
M	cmd/podman/containers/start.go
M	cmd/podman/containers/stats.go
M	cmd/podman/containers/stop.go
M	cmd/podman/containers/top.go
M	cmd/podman/containers/unmount.go
M	cmd/podman/containers/unpause.go
M	cmd/podman/containers/update.go
M	cmd/podman/containers/wait.go
M	cmd/podman/utils/utils.go
M	libpod/options.go
M	libpod/runtime_ctr.go
M	pkg/domain/filters/containers.go
M	test/system/030-run.bats
Falling back to patching base and 3-way merge...
Auto-merging test/system/030-run.bats
CONFLICT (content): Merge conflict in test/system/030-run.bats
Auto-merging pkg/domain/filters/containers.go
Auto-merging libpod/runtime_ctr.go
Auto-merging libpod/options.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Allow '/' to prefix container names to match Docker
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick v4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@vrothberg
Copy link
Member

This is already in the v4.4 branch

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 11, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker ps --fitler 'name=^/...$ (leading slash in container name) does not work with Podman macOS helper
8 participants