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.

Sign up for GitHub

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

Jump to bottom

critest: remove dependency of source code #261

Merged
merged 3 commits into from
Mar 15, 2018
Merged

critest: remove dependency of source code #261

merged 3 commits into from
Mar 15, 2018

Conversation

feiskyer
Copy link
Member

critest requires Go and cri-tools source code to build e2e.test and benchmark.test. This makes critest hard to install and need to switch source code branches when running old versions.

This PR removes this requirement and makes only a single critest binary required for CRI validation tests. After this PR, downloading from a release is encouraged to install critest.

It will also help to add validation tests for windows containers #223 .

/cc @Random-Liu @mrunalp @runcom @resouer

@k8s-ci-robot
Copy link
Contributor

@feiskyer: GitHub didn't allow me to request PR reviews from the following users: resouer.

Note that only kubernetes-incubator members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

critest requires Go and cri-tools source code to build e2e.test and benchmark.test. This makes critest hard to install and need to switch source code branches when running old versions.

This PR removes this requirement and makes only a single critest binary required for CRI validation tests. After this PR, downloading from a release is encouraged to install critest.

It will also help to add validation tests for windows containers #223 .

/cc @Random-Liu @mrunalp @runcom @resouer

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 28, 2018
@feiskyer feiskyer added this to the v1.0.0-beta.0 milestone Feb 28, 2018
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 28, 2018
feiskyer
feiskyer commented Feb 28, 2018
View reviewed changes
docs/validation.md

```sh
go get github.com/kubernetes-incubator/cri-tools/cmd/critest
wget https://github.com/kubernetes-incubator/cri-tools/releases/download/v1.0.0-beta.0/critest-v1.0.0-beta.0-linux-amd64.tar.gz
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that the file does not exist yet, it should be uploaded after beta version released.

@Random-Liu Random-Liu self-assigned this Feb 28, 2018
@Random-Liu
Copy link
Contributor

This is what I want. I just don't know whether we have time to finish this, so I didn't file the issue.
Thanks!

@feiskyer feiskyer mentioned this pull request Mar 1, 2018
Closed
@feiskyer
Copy link
Member Author

ping @Random-Liu

mrunalp
mrunalp approved these changes Mar 14, 2018
View reviewed changes
Copy link
Member

@mrunalp mrunalp left a comment

Choose a reason for hiding this comment

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

LGTM

@umohnani8
Copy link
Contributor

Tested it and worked for me! LGTM

@feiskyer
Copy link
Member Author

Rebased to master branch

@Random-Liu
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2018
@Random-Liu Random-Liu merged commit f94688c into kubernetes-sigs:master Mar 15, 2018
@Random-Liu
Copy link
Contributor

Random-Liu commented Mar 15, 2018 •
edited
Loading

@feiskyer The parallel mode doesn't work. Only 8 test runs after this change.

Random Seed: 1521084457 - Will randomize all specs
Parallel test node 1/8.
...
Ran 8 of 8 Specs in 9.863 seconds
SUCCESS! -- 8 Passed | 0 Failed | 0 Pending | 0 Skipped PASS

It seems that running test in parallel still need ginkgo.

@feiskyer feiskyer deleted the critest branch March 15, 2018 06:46
@Random-Liu Random-Liu mentioned this pull request Mar 16, 2018
Merged
@feiskyer feiskyer mentioned this pull request Mar 19, 2018
Merged
@Random-Liu Random-Liu mentioned this pull request Mar 20, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrunalp mrunalp mrunalp approved these changes

@Random-Liu Random-Liu Awaiting requested review from Random-Liu

@runcom runcom Awaiting requested review from runcom

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.0.0-beta.0
Development

Successfully merging this pull request may close these issues.
5 participants
@feiskyer @k8s-ci-robot @Random-Liu @umohnani8 @mrunalp