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

refactor: correct hard coding in several shell script #1452

Merged
merged 2 commits into from
Jun 4, 2018

Conversation

zhuangqh
Copy link
Contributor

@zhuangqh zhuangqh commented Jun 1, 2018

Signed-off-by: zhuangqh [email protected]

Ⅰ. Describe what this PR did

Correct hard coding in several shell script, using configurable variable instead.
Fix a minor bug in #1447

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

hack/make.sh Outdated
@@ -9,9 +9,13 @@ DIR="$( cd "$( dirname "$0" )/.." && pwd )"
cd "$DIR/"

CONTAINERD_VERSION=
REQUIRED_CONTAINERD_VERSION="1.0.3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering if we could directly use CONTAINERD_VERSION rather than REQUIRED_CONTAINERD_VERSION ?

Or add some annotation for both two variables and tell code readers the difference between them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CONTAINERD_VERSION is the version of containerd installed in this machine, getting from command containerd -v .... If CONTAINERD_VERSION not equals to required version, this script will install the required version.
Maybe we can change CONTAINERD_VERSION to CURRENT_CONTAINERD_VERSION for better semantics? @allencloud

Copy link
Collaborator

Choose a reason for hiding this comment

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

From your explanation, I have got what you mean. While I think it is OK for the variables, but there is still some comment required in the script. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. First reading these code may be confused, I've added a comment in next commit. @allencloud

@allencloud
Copy link
Collaborator

It happens to be error exists in critools testing, @starnop . Actually it says that there is one failure in the critools testing. However, I think it is quite hard for me to judge which one is failing. Is there any way for the critools to show explicit error case.

In addition, I retrigger the critools testing to check if this is a flaky test. I have to say in the future we need to pay more attention on the flaky test in CRI part. It is quite important to be related with the stability of Pouchd.

@allencloud allencloud changed the title enhance: correct hard coding in several shell script refactor: correct hard coding in several shell script Jun 4, 2018
@codecov-io
Copy link

codecov-io commented Jun 4, 2018

Codecov Report

Merging #1452 into master will decrease coverage by 1.5%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1452      +/-   ##
==========================================
- Coverage   40.05%   38.54%   -1.51%     
==========================================
  Files         249      250       +1     
  Lines       16179    17057     +878     
==========================================
+ Hits         6480     6575      +95     
- Misses       8880     9635     +755     
- Partials      819      847      +28
Impacted Files Coverage Δ
apis/server/container_bridge.go 53.8% <0%> (-29.01%) ⬇️
daemon/mgr/container_utils.go 51.07% <0%> (-28.32%) ⬇️
apis/server/volume_bridge.go 63.1% <0%> (-23.38%) ⬇️
apis/server/network_bridge.go 44.87% <0%> (-18.44%) ⬇️
daemon/mgr/container.go 36.27% <0%> (-13.73%) ⬇️
apis/server/utils.go 49.09% <0%> (-12.03%) ⬇️
ctrd/image.go 76.57% <0%> (-2.86%) ⬇️
daemon/containerio/container_io.go 57.79% <0%> (-1.95%) ⬇️
pkg/system/cgroup.go 87.09% <0%> (ø)
daemon/mgr/network.go 69.28% <0%> (+0.49%) ⬆️

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jun 4, 2018
@allencloud allencloud merged commit ab26bf6 into AliyunContainerService:master Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor LGTM one maintainer or community participant agrees to merge the pull reuqest. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants