ctr/run: flags --detach and --rm cannot be specified together #7929
Conversation
|
Hi @Iceber. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
Iceber
changed the title
| return err | ||
| } | ||
| if context.Bool("rm") && !detach { | ||
| if rm && !detach { |
There was a problem hiding this comment.
If we supplied rm we should know detach is false as we'd exit on line 159 now otherwise
There was a problem hiding this comment.
Although the upper logic provides some checks, I think the prerequisite for perform defer container.Delete(ctx, containerd.WithSnapshotCleanup) is strict rm && !detach.
For robustness I did not remove the judgment on detach here, what do you think?
Of course, for the sake of code brevity, I think it's fine to just judge rm
There was a problem hiding this comment.
It was mainly a nit, so I'm fine with this!
estesp
left a comment
estesp
left a comment
There was a problem hiding this comment.
LGTM w/2 minor nits about the message text. Not critical, but maybe useful to keep messages more terse
|
I've re-run the Cirrus CI checks a few times and they continue to fail on the same error, while other PRs are succeeding. The change in this PR is totally unrelated, so I'm not sure why. I do see that the PR branch is 57 commits behind master; do you want to rebase the PR on current |
Signed-off-by: Iceber Gu <[email protected]>
|
/retest |
|
@Iceber: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
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. |
|
I rebase the upstream/main and the latest failed test |
samuelkarp
added
cherry-picked/1.6.x
… main Updates ADO containerd fork-external/main with upstream containerd main @commit hash: f9f8455 containerd@f9f8455 Additional changes in the PR: - update linux container image version - remove copying LICENSE file in shared-build-stages.yml - additions to .gdnsuppress Related work items: containerd#5674, containerd#7393, containerd#7661, containerd#7685, containerd#7810, containerd#7850, containerd#7861, containerd#7879, containerd#7880, containerd#7881, containerd#7882, containerd#7883, containerd#7886, containerd#7887, containerd#7888, containerd#7891, containerd#7892, containerd#7893, containerd#7894, containerd#7903, containerd#7904, containerd#7905, containerd#7906, containerd#7907, containerd#7908, containerd#7911, containerd#7913, containerd#7914, containerd#7917, containerd#7925, containerd#7927, containerd#7928, containerd#7929, containerd#7932, containerd#7935, containerd#7943, containerd#7946, containerd#7948, containerd#7957, containerd#7958, containerd#7960, containerd#7963, containerd#7969, containerd#7970, containerd#7973
…/main Update fork-external/main with upstream containerd/containerd/main at commit hash 3d32da8 Related work items: containerd#5674, containerd#7129, containerd#7393, containerd#7661, containerd#7685, containerd#7810, containerd#7850, containerd#7861, containerd#7882, containerd#7883, containerd#7886, containerd#7891, containerd#7892, containerd#7893, containerd#7903, containerd#7904, containerd#7905, containerd#7906, containerd#7907, containerd#7908, containerd#7911, containerd#7913, containerd#7914, containerd#7917, containerd#7925, containerd#7927, containerd#7928, containerd#7929, containerd#7932, containerd#7935, containerd#7943, containerd#7946, containerd#7948, containerd#7957, containerd#7958, containerd#7959, containerd#7960, containerd#7963, containerd#7968, containerd#7969, containerd#7970, containerd#7973, containerd#7985, containerd#7987, containerd#7994, containerd#8005
5 participants
fix: #7923