Navigation Menu

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

test: api_ulimit_test: add test for ulimit #2328

Merged
merged 1 commit into from Dec 27, 2018

Conversation

chuanchang
Copy link
Contributor

This patch will cover ulimit related testing.

Signed-off-by: Alex Jia chuanchang.jia@gmail.com

Ⅰ. Describe what this PR did

As summary.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

Ⅳ. Describe how to verify it

go test

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Oct 19, 2018

Codecov Report

Merging #2328 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2328      +/-   ##
=========================================
+ Coverage   69.07%   69.1%   +0.03%     
=========================================
  Files         279     279              
  Lines       18834   18834              
=========================================
+ Hits        13009   13016       +7     
+ Misses       4360    4349      -11     
- Partials     1465    1469       +4
Flag Coverage Δ
#criv1alpha1test 31.37% <ø> (ø) ⬆️
#criv1alpha2test 35.67% <ø> (-0.06%) ⬇️
#integrationtest 41.22% <ø> (+0.04%) ⬆️
#nodee2etest 32.74% <ø> (-0.12%) ⬇️
#unittest 26.89% <ø> (+0.11%) ⬆️
Impacted Files Coverage Δ
pkg/streams/utils.go 82.14% <0%> (-9.53%) ⬇️
cri/v1alpha2/cri_wrapper.go 64.4% <0%> (-1.2%) ⬇️
daemon/mgr/spec_linux.go 76.74% <0%> (-1%) ⬇️
cri/v1alpha2/cri.go 67.57% <0%> (-0.62%) ⬇️
daemon/mgr/container.go 59.21% <0%> (ø) ⬆️
ctrd/container.go 59.28% <0%> (+0.39%) ⬆️
ctrd/image.go 78.18% <0%> (+2.27%) ⬆️
apis/opts/config/ulimit.go 79.16% <0%> (+79.16%) ⬆️

@CLAassistant
Copy link

CLAassistant commented Oct 19, 2018

CLA assistant check
All committers have signed the CLA.

@chuanchang
Copy link
Contributor Author

@Ace-Tang how to understand the following issue? thanks!

[Fail] [k8s.io] Security Context bucket [It] runtime should support RunAsUserName 
/home/travis/gopath/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/container.go:467

apis/opts/config/ulimit.go Outdated Show resolved Hide resolved
@Ace-Tang
Copy link
Contributor

Ace-Tang commented Oct 23, 2018

@chuanchang , I will check the issue.

It("runtime should support RunAsUserName", func() {
            By("create pod")
            podID, podConfig = framework.CreatePodSandboxForContainer(rc)

            By("create container for security context RunAsUser")
            containerID, expectedLogMessage := createRunAsUserNameContainer(rc, ic, podID, podConfig, "container-with-RunAsUserName-test-")

            By("start container")
            startContainer(rc, containerID)
            Eventually(func() runtimeapi.ContainerState {
                return getContainerStatus(rc, containerID).State
            }, time.Minute, time.Second*4).Should(Equal(runtimeapi.ContainerState_CONTAINER_RUNNING))

            By("verify RunAsUserName for container")
            command := []string{"id", "-nu"}
            verifyExecSyncOutput(rc, containerID, command, expectedLogMessage)
        })   

Seems validate pouch run -u user

@zhuangqh
Copy link
Contributor

zhuangqh commented Nov 5, 2018

ping @chuanchang any update/response of this PR?

@zhuangqh
Copy link
Contributor

zhuangqh commented Dec 25, 2018

After offline discussion with @chuanchang . We are going to keep the source code unchanged(remove the sort part) and remove the test for String() interface because this part of test is meaningless and so complicated to test.

String() interface translate a []string to string by fmt.Sprintf("%v", ...)

cc @allencloud @fuweid @Ace-Tang

please move this on @chuanchang , thanks!

@chuanchang
Copy link
Contributor Author

please move this on @chuanchang , thanks!

sure.

Signed-off-by: Alex Jia <chuanchang.jia@gmail.com>
Copy link
Contributor

@zhuangqh zhuangqh left a comment

Choose a reason for hiding this comment

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

LGTM

@zhuangqh zhuangqh merged commit 7eb077e into AliyunContainerService:master Dec 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants