Skip to content

Cleanup code#5383

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
wzshiming:clean/process-io
Apr 20, 2021
Merged

Cleanup code#5383
crosbymichael merged 1 commit intocontainerd:masterfrom
wzshiming:clean/process-io

Conversation

@wzshiming
Copy link
Copy Markdown
Contributor

@wzshiming wzshiming commented Apr 19, 2021 •
edited
Loading

Clean up the code, move runtime.NewBinaryCmd to process.NewBinaryCmd and use process.NewBinaryCmd directly

containerd/runtime/io.go

Lines 25 to 45 in bbbd851

// NewBinaryCmd returns a Cmd to be used to start a logging binary.
// The Cmd is generated from the provided uri, and the container ID and
// namespace are appended to the Cmd environment.
func NewBinaryCmd(binaryURI *url.URL, id, ns string) *exec.Cmd {
var args []string
for k, vs := range binaryURI.Query() {
args = append(args, k)
if len(vs) > 0 {
args = append(args, vs[0])
}
}
cmd := exec.Command(binaryURI.Path, args...)
cmd.Env = append(cmd.Env,
"CONTAINER_ID="+id,
"CONTAINER_NAMESPACE="+ns,
)
return cmd
}

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Apr 19, 2021

Hi @wzshiming. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 19, 2021

Build succeeded.

fuweid
fuweid reviewed Apr 19, 2021
View reviewed changes
Comment thread pkg/process/io.go Outdated
Copy link
Copy Markdown
Member

@fuweid fuweid Apr 19, 2021

Choose a reason for hiding this comment

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

Well. For the dependency, the runtime package depends on pkg/process package. :) The duplicate code is not bad in some case.

Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Apr 19, 2021

Choose a reason for hiding this comment

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

the runtime package depends on pkg/process package

Doesn't seem so

Copy link
Copy Markdown
Member

@fuweid fuweid Apr 19, 2021

Choose a reason for hiding this comment

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

The runtime package is almost used to define interface and the runtime/{v1,v2}/{runc,shim} packages share that pkg/process. If possible, I would like to see the NewBinaryCmd in pkg instead of runtime

@wzshiming
Cleanup code
7966a66 
Signed-off-by: Shiming Zhang <[email protected]>
@wzshiming wzshiming requested review from AkihiroSuda and fuweid April 19, 2021 09:01
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 19, 2021

Build succeeded.

@crosbymichael crosbymichael merged commit a3fe5c8 into containerd:master Apr 20, 2021
@wzshiming wzshiming deleted the clean/process-io branch April 21, 2021 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crosbymichael crosbymichael crosbymichael approved these changes

@mxpv mxpv mxpv approved these changes

@fuweid fuweid Awaiting requested review from fuweid

@AkihiroSuda AkihiroSuda Awaiting requested review from AkihiroSuda

Assignees

No one assigned

Labels

needs-ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@wzshiming @k8s-ci-robot @crosbymichael @mxpv @AkihiroSuda @fuweid