Skip to content

Run shellSchek #17

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

Merged
merged 2 commits into from
Aug 9, 2020
Merged

Run shellSchek #17

merged 2 commits into from
Aug 9, 2020

Conversation

MichelDiz
Copy link
Contributor

@MichelDiz MichelDiz commented Aug 9, 2020

Passing shell check lint.


This change is Reviewable

Copy link

@darkn3rd darkn3rd left a comment

Choose a reason for hiding this comment

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

I am curious about the quoting style, as I usually only see this if trying to capture a newline. Dropped some questions. Nothing blocking though.

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @MichelDiz)


getdgraph.sh, line 28 at r1 (raw file):

Quoted 4 lines of code…
acceptLower=`echo "$ACCEPT_LICENSE" | dd  conv=lcase 2> /dev/null`
systemdLower=`echo "$INSTALL_IN_SYSTEMD" | dd  conv=lcase 2> /dev/null`
ACCEPT_LICENSE=${acceptLower:-n}
INSTALL_IN_SYSTEMD=${systemdLower:-n}

Why is it necessary to subshell to lower or upper case?

ACCEPT_LICENSE=${ACCEPT_LICENSE,,:-n}
INSTALL_IN_SYSTEMD=${INSTALL_IN_SYSTEMD,,:-n}

getdgraph.sh, line 156 at r1 (raw file):

checksum_link=$( echo https://github.com/dgraph-io/dgraph/releases/download/"$tag"/"$checksum_file")

Not really understanding why you do this in a sub-shell. Did tag or checksum_file have new lines or tabs in the file names?

why not:

checksum_link=echo https://github.com/dgraph-io/dgraph/releases/download/$tag/$checksum_file"

getdgraph.sh, line 200 at r1 (raw file):

tar -C "$temp_dir" -xzf /tmp/"$tar_file"

Did these locations have spaces in them?


getdgraph.sh, line 220 at r1 (raw file):

		$sudo_cmd mv "$temp_dir"/* /usr/local/bin/.
		rm "/tmp/""$tar_file";
		rm -rf "$temp_dir"

I understand you need the glob outside fo the quote, but why quote-barrier $temp_dir, would this be locations with spaces in the directory name?

Copy link
Contributor Author

@MichelDiz MichelDiz left a comment

Choose a reason for hiding this comment

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

If you paste the script on https://www.shellcheck.net/ you'll see all recommendations. I haven't followed all of them.

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @darkn3rd)


getdgraph.sh, line 28 at r1 (raw file):

Previously, darkn3rd (Joaquin Menchaca) wrote…
acceptLower=`echo "$ACCEPT_LICENSE" | dd  conv=lcase 2> /dev/null`
systemdLower=`echo "$INSTALL_IN_SYSTEMD" | dd  conv=lcase 2> /dev/null`
ACCEPT_LICENSE=${acceptLower:-n}
INSTALL_IN_SYSTEMD=${systemdLower:-n}

Why is it necessary to subshell to lower or upper case?

ACCEPT_LICENSE=${ACCEPT_LICENSE,,:-n}
INSTALL_IN_SYSTEMD=${INSTALL_IN_SYSTEMD,,:-n}

I don't remember exactly why, but I just followed a recommendation. But not sure what you mean, is there some issue? BTW this isn't part of this PR and it was long ago.


getdgraph.sh, line 156 at r1 (raw file):

Previously, darkn3rd (Joaquin Menchaca) wrote…
checksum_link=$( echo https://github.com/dgraph-io/dgraph/releases/download/"$tag"/"$checksum_file")

Not really understanding why you do this in a sub-shell. Did tag or checksum_file have new lines or tabs in the file names?

why not:

checksum_link=echo https://github.com/dgraph-io/dgraph/releases/download/$tag/$checksum_file"

That was a recommendation from https://www.shellcheck.net/


getdgraph.sh, line 200 at r1 (raw file):

Previously, darkn3rd (Joaquin Menchaca) wrote…
tar -C "$temp_dir" -xzf /tmp/"$tar_file"

Did these locations have spaces in them?

Spaces? which spaces?


getdgraph.sh, line 220 at r1 (raw file):

Previously, darkn3rd (Joaquin Menchaca) wrote…
		$sudo_cmd mv "$temp_dir"/* /usr/local/bin/.
		rm "/tmp/""$tar_file";
		rm -rf "$temp_dir"

I understand you need the glob outside fo the quote, but why quote-barrier $temp_dir, would this be locations with spaces in the directory name?

Again, that's from https://www.shellcheck.net/

@MichelDiz MichelDiz merged commit f44abc8 into master Aug 9, 2020
@MichelDiz MichelDiz deleted the micheldiz/shellcheck branch August 9, 2020 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants