-
Notifications
You must be signed in to change notification settings - Fork 944
[bug]: roolback the code about CRI of version v1alpha1 #1755
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
[bug]: roolback the code about CRI of version v1alpha1 #1755
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1755 +/- ##
========================================
+ Coverage 56.3% 62.1% +5.8%
========================================
Files 200 200
Lines 15657 15565 -92
========================================
+ Hits 8816 9667 +851
+ Misses 5745 4651 -1094
- Partials 1096 1247 +151
|
83cbdbf to
03a5701
Compare
|
it seems that you forgot to change the Makefile and .travis.yml. We need four jobs here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change Makefile to support v1aphla1 CI
| CRITEST_VERSION="1.0.0-alpha.0" | ||
| else | ||
| CRITEST_VERSION="1.0.0-beta.0" | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add the blank line.
| if [[ "${cri_runtime}" == "v1alpha1" ]]; then | ||
| echo "run v1alpha1 test" | ||
| critest --runtime-endpoint=${POUCH_SOCK} \ | ||
| --focus="${CRI_FOCUS}" --ginkgo-flags="--skip=\"${CRI_SKIP}\"" validation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add indent....
| else | ||
| echo "run v1alpha2 test" | ||
| critest --runtime-endpoint=${POUCH_SOCK} \ | ||
| --ginkgo.focus="${CRI_FOCUS}" --ginkgo.skip="${CRI_SKIP}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add indent...
| local cri_runtime cmd flags | ||
| cri_runtime=$1 | ||
| cmd="pouchd-integration" | ||
| flags=" -test.coverprofile=${coverage_profile} DEVEL" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use different coverage-profile for alpha1 otherwise the alpha2 will override the result.
|
|
||
| main() { | ||
| integration::install_cni | ||
| cri_runtime="v1alpha1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please run integration::run_cri_test "${cri_runtime}" don't use global cri_runtime. If you have to, please use local to consume the cri_runtime to avoid the global variable pollution
| { | ||
| "checksumSHA1": "oRQMrak6qTTdEUnRUMtVpzO9T/A=", | ||
| "path": "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime", | ||
| "revision": "18758f502c4a249bf999f34e4a115ec9881cd9f2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YaoZengzeng it's correct about the revision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's correct.
92de646 to
321a6fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically, the change is ok to me. @YaoZengzeng Please help to check again. Thanks
|
|
||
| # daemon cri integration coverage profile | ||
| coverage_profile="${REPO_BASE}/coverage/integration_daemon_cri_${cri_runtime}_profile.out" | ||
| echo "${coverage_profile}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this line. we can use bash -x to check the value~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
999afc8 to
797aa95
Compare
|
LGTM. |
Makefile
Outdated
| ./hack/testing/run_daemon_cri_integration.sh v1alpha2 | ||
|
|
||
| .PHONY: test | ||
| test: unit-test integration-test cri-test ## run the unit-test, integration-test and cri-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cri-test should be changed
| critest --runtime-endpoint=${POUCH_SOCK} \ | ||
| --ginkgo.focus="${CRI_FOCUS}" --ginkgo.skip="${CRI_SKIP}" | ||
| if [[ "${cri_runtime}" == "v1alpha1" ]]; then | ||
| echo "run v1alpha1 test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the above line start pouch daemon cri integration test... to show the version, like
start pouch daemon cri-${version} integration test.... Does it make senses to you?
| critest --runtime-endpoint=${POUCH_SOCK} \ | ||
| --focus="${CRI_FOCUS}" --ginkgo-flags="--skip=\"${CRI_SKIP}\"" validation | ||
| else | ||
| echo "run v1alpha2 test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this line.
797aa95 to
ea78a5d
Compare
Signed-off-by: Starnop <[email protected]>
ea78a5d to
10bbe90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Starnop [email protected]
Ⅰ. Describe what this PR did
For CRI expansion requirements,the code about CRI of version v1alpha1 fail the test. Meanwhile,we only need to expand v1alpha2. So we roolback the code about CRI of version v1alpha1.
Ⅱ. Does this pull request fix one issue?
none
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews