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

fix: correct shell script format via shellcheck reports #1447

Merged
merged 2 commits into from
Jun 1, 2018

Conversation

zhuangqh
Copy link
Contributor

Ⅰ. Describe what this PR did

This PR help pouch to pass the ShellCheck (https://github.com/koalaman/shellcheck). There is a short report.

SC2086 Double quote to prevent globbing and word splitting 94 fail(s)
SC2046 Quote this to prevent word splitting 8 fail(s)
SC2034 POUCH_SOCK appears unused. Verify it or export it 7 fail(s)
SC2039 In POSIX sh, 'pushd' is not supported 6 fail(s)
SC2148 Tips depend on target shell and yours is unknown. Add a shebang 5 fail(s)
SC2112 'function' keyword is non-standard. Delete it 4 fail(s)
SC2016 Expressions don't expand in single quotes, use double quotes for that 3 fail(s)
SC2006 Use $(..) instead of legacy .. 2 fail(s)
SC2129 Consider using { cmd1; cmd2; } >> file instead of individual redirects 1 fail(s)
SC2071 < is for string comparisons. Use -lt instead 1 fail(s)
SC2068 Double quote array expansions, otherwise they're like $* and break on spaces 1 fail(s)
SC2064 Use single quotes, otherwise this expands now rather than when signalled 1 fail(s)

You can find the detailed rules here

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

ShellCheck don't report error.

find ./ -name "*.sh" | grep -v vendor | grep -v extra | xargs shellcheck

Shell script still works fine.

Ⅴ. Special notes for reviews

In rule SC2086

When quoting composite arguments, make sure to exclude globs and brace expansions, which lose their special meaning in double quotes: "$HOME/$dir/src/*.c" will not expand, but "$HOME/$dir/src"/*.c will.

@pouchrobot pouchrobot added kind/bug This is bug report for project size/XL labels May 31, 2018
@fuweid
Copy link
Contributor

fuweid commented May 31, 2018

Nice!

@zhuangqh
Copy link
Contributor Author

related to #1437

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #1447       +/-   ##
==========================================
+ Coverage   16.26%   37.5%   +21.24%     
==========================================
  Files         206     254       +48     
  Lines       13759   17368     +3609     
==========================================
+ Hits         2238    6514     +4276     
+ Misses      11365   10023     -1342     
- Partials      156     831      +675
Impacted Files Coverage Δ
cli/main.go 0% <0%> (ø) ⬆️
client/container_wait.go
cli/wait.go
cri/stream/errors.go 0% <0%> (ø)
cri/stream/httpstream/httpstream.go 0% <0%> (ø)
storage/volume/error/errors.go 28.57% <0%> (ø)
pkg/system/os.go 75% <0%> (ø)
cri/stream/remotecommand/httpstream.go 0% <0%> (ø)
cri/stream/httpstream/spdy/connection.go 0% <0%> (ø)
storage/volume/modules/local/local.go 68.57% <0%> (ø)
... and 115 more

--description 'Pouch is an open-source project created by Alibaba Group to promote the container technology movement.

Pouchs vision is to advance container ecosystem and promote container standards OCI, so that container technologies become the foundation for application development in the Cloud era.
Pouch'"'"'s vision is to advance container ecosystem and promote container standards OCI, so that container technologies become the foundation for application development in the Cloud era.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think this change is correct.

Copy link
Contributor Author

@zhuangqh zhuangqh May 31, 2018

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I see. This is not just doc, but a string in shell script. My fault.

tar xf $TMP/containerd-1.0.3.linux-amd64.tar.gz -C $TMP &&
cp -f $TMP/bin/* /usr/local/bin/
https://github.com/containerd/containerd/releases/download/v1.0.3/containerd-1.0.3.linux-amd64.tar.gz -P "$TMP"
tar xf "$TMP/containerd-1.0.3.linux-amd64.tar.gz" -C "$TMP" &&
Copy link
Collaborator

@allencloud allencloud May 31, 2018

Choose a reason for hiding this comment

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

I think in the future we need to make this version configurable at the head of this file rather than hard coding here. And all the 1.0.3 things should use the variable.

@@ -48,19 +47,19 @@ function build_pouch()
{
# install containerd
echo "Downloading containerd."
wget --quiet https://github.com/containerd/containerd/releases/download/v1.0.3/containerd-1.0.3.linux-amd64.tar.gz -P $TMP
tar xf $TMP/containerd-1.0.3.linux-amd64.tar.gz -C $TMP && cp -f $TMP/bin/* $BINDIR/
wget --quiet https://github.com/containerd/containerd/releases/download/v1.0.3/containerd-1.0.3.linux-amd64.tar.gz -P "$TMP"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also this place, there is hard coding

wget --quiet https://github.com/alibaba/runc/releases/download/v1.0.0-rc4-1/runc.amd64 -P $BINDIR/
chmod +x $BINDIR/runc.amd64
mv $BINDIR/runc.amd64 $BINDIR/runc
wget --quiet https://github.com/alibaba/runc/releases/download/v1.0.0-rc4-1/runc.amd64 -P "$BINDIR/"
Copy link
Collaborator

Choose a reason for hiding this comment

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

also here, hard coding

@allencloud
Copy link
Collaborator

LGTM since I take a quick review of this. However, I still wish to invite @ZouRui89 to take a thorough review of this. Thanks a lot.

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label May 31, 2018
@allencloud allencloud merged commit 74e5ac7 into AliyunContainerService:master Jun 1, 2018
wget --quiet https://github.com/containerd/containerd/releases/download/v1.0.3/containerd-1.0.3.linux-amd64.tar.gz -P $TMP
tar xf $TMP/containerd-1.0.3.linux-amd64.tar.gz -C $TMP && cp -f $TMP/bin/* $BINDIR/
wget --quiet https://github.com/containerd/containerd/releases/download/v1.0.3/containerd-1.0.3.linux-amd64.tar.gz -P "$TMP"
tar xf "$TMP/containerd-1.0.3.linux-amd64.tar.gz -C $TMP && cp -f $TMP/bin/* $BINDIR/"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should double quote one by one, or won't work.😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix this in another PR, together with the problem of hardcode.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zhuangqh cool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants