Skip to content

Bash scripts; use double brackets, fix bare variables, add quotes#38424

Merged
vdemeester merged 1 commit intomoby:masterfrom
thaJeztah:bash_da_bash_bash_bash
Dec 24, 2018
Merged

Bash scripts; use double brackets, fix bare variables, add quotes#38424
vdemeester merged 1 commit intomoby:masterfrom
thaJeztah:bash_da_bash_bash_bash

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Dec 23, 2018

These scripts explicitly use Bash, so we should be able to use [[ instead of [ (which seems to be recommended).

Also added curly brackets to some bare variables, and quoted some paths.

This makes my IDE a bit more silent :-)

@thaJeztah
Bash scripts; use double brackets, fix bare variables, add quotes
297b30d 
These scripts explicitly use Bash, so we should be able to use
`[[` instead of `[` (which seems to be recommended).

Also added curly brackets to some bare variables, and quoted some paths.

This makes my IDE a bit more silent :-)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah requested a review from tianon as a code owner December 23, 2018 23:06
@thaJeztah
Copy link
Member Author

thaJeztah commented Dec 23, 2018

@tianon @albers PTAL; happy to close if these changes don't make sense

@codecov
Copy link

codecov bot commented Dec 24, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@d147fe0). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #38424   +/-   ##
=========================================
  Coverage          ?   36.55%           
=========================================
  Files             ?      608           
  Lines             ?    45036           
  Branches          ?        0           
=========================================
  Hits              ?    16465           
  Misses            ?    26293           
  Partials          ?     2278

1 similar comment
@codecov
Copy link

codecov bot commented Dec 24, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@d147fe0). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #38424   +/-   ##
=========================================
  Coverage          ?   36.55%           
=========================================
  Files             ?      608           
  Lines             ?    45036           
  Branches          ?        0           
=========================================
  Hits              ?    16465           
  Misses            ?    26293           
  Partials          ?     2278

vdemeester
vdemeester approved these changes Dec 24, 2018
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@vdemeester vdemeester merged commit 052a20f into moby:master Dec 24, 2018
@thaJeztah thaJeztah deleted the bash_da_bash_bash_bash branch December 24, 2018 13:06
@tianon
Copy link
Member

tianon commented Jan 9, 2019

Ah, wish I'd dug in and seen this sooner. 😞 (Sorry!)

See https://serverfault.com/a/705144 for a great (very extensive) post which outlines some of my own thoughts on this topic.

tianon added a commit to docker-library/docker that referenced this pull request Jan 9, 2019
@tianon
Pin "dindLatest"
d7d086d 
We use this script via /bin/sh because it hasn't had any Bashisms lately (until moby/moby#38424), so we need to stop updating (at least for now).
@thaJeztah
Copy link
Member Author

thaJeztah commented Jan 10, 2019

@tianon want me to revert the double-bracket changes? I know we tried to avoid Bashisms in the past, but I noticed these files used /bin/bash

@tianon
Copy link
Member

tianon commented Jan 10, 2019

I would certainly appreciate the double brackets going away 😅

In the docker:dind images we use the dind script here via /bin/sh which works OK until this change (maybe that script ought to be explicitly /bin/sh anyhow? lol)

This was referenced Jan 10, 2019
Merged
Merged
@thaJeztah thaJeztah mentioned this pull request Feb 25, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Aug 19, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Aug 26, 2019
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@tianon tianon Awaiting requested review from tianon tianon is a code owner

+1 more reviewer

@vdemeester vdemeester vdemeester approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/testing process/cherry-picked status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@thaJeztah @tianon @vdemeester @AkihiroSuda @GordonTheTurtle