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

bash 3.x compatibility; avoid mapfile #48

Merged
merged 3 commits into from
Jun 29, 2020
Merged

bash 3.x compatibility; avoid mapfile #48

merged 3 commits into from
Jun 29, 2020

Conversation

pda
Copy link
Contributor

@pda pda commented Jun 26, 2020

Use of mapfile was introduced in da2472f (to fix a shellcheck) and 509678c (to support awscli v2). mapfile was introduced in bash 4.x, but there are many systems that ship with bash 3.x (perhaps just macOS, but there's a lot of macOS systems 😅).

This patch takes advice from shellcheck at https://github.com/koalaman/shellcheck/wiki/SC2207 to use bash 3.x compatible read -a to remove the dependency on mapfile while also avoiding the previous shellcheck warnings.

Fixes #46, closes #47.

Ping: @pdbreen @ryansdwilson @kkolk

@pda pda requested a review from jayco June 26, 2020 06:39
@jayco jayco requested a review from toolmantim June 26, 2020 06:50
Copy link

@jayco jayco left a comment

Choose a reason for hiding this comment

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

This looks good! Would be awesome to get @toolmantim 👀 on this also.

@pda pda self-assigned this Jun 26, 2020
@pda pda requested a review from jayco June 26, 2020 07:00
@toolmantim
Copy link
Contributor

@pda bash is always beyond my bash!

I think we might have to remove the integration test for now, on our new untrusted VM world

@pda
Copy link
Contributor Author

pda commented Jun 26, 2020

Removing that acceptance test in #49

@brett-anderson
Copy link

brett-anderson commented Jun 26, 2020

Thanks for jumping on this fix Paul, I tried running this this morning with your patch and got:

 line 134: account_ids[*]: unbound variable

this is what my pipeline config looks like:

- ecr#5b8f36f57f756f65b1b4fbe265e99e91844996d0:
          login: true
          no-include-email: true

Any idea why account_ids is unbound?

Edit:

I added my account ID and it ran successfully.

@pdbreen
Copy link

pdbreen commented Jun 26, 2020

Thanks @pda - and very timely. Turned out much of my day was working on pipelines and this helped speed up the change/test cycle time by an amazing amount by testing local. Other than needing to set account_ids like Brett mentioned, worked great.

@pda
Copy link
Contributor Author

pda commented Jun 27, 2020

Interesting… it appears bash 3.x treats ${array[*]} as unbound if the array exists but is empty, while newer bash works as expected.

for ver in 3.2 4.4 5.0; do
    docker run --rm bash:$ver bash -u -c 'echo $BASH_VERSION; a=(); echo ${a[*]} ok';
done

# 3.2.57(1)-release
# bash: a[*]: unbound variable

# 4.4.23(1)-release
# ok

# 5.0.17(1)-release
# ok

I've pushed a change to check for emptiness in a different way:

-  if [[ -z ${account_ids[*]} ]]; then
+  if [[ ${#account_ids[@]} -eq 0 ]]; then

The semantics are slightly different if the array contains a single empty string:

set -u
a=("")
echo "count: ${#a[@]}"
if [[ -z "${a[*]}" ]]; then echo "empty"; else echo "non-empty"; fi

# count: 1
# empty

But I don't think that's a problem for us 🤞🏼

@pda
Copy link
Contributor Author

pda commented Jun 27, 2020

Actually I ended up using a combination of the two approaches, to keep the old behaviour but avoid the bash 3.x error:

-  if [[ -z ${account_ids[*]} ]]; then                                  # before this PR
-  if [[ ${#account_ids[@]} -eq 0 ]]; then                              # earlier in this PR
+  if [[ ${#account_ids[@]} -eq 0 || -z "${account_ids[*]}" ]]; then    # combo

It only reaches the -z "${account_ids[*]}" part if the array is not empty, so that avoids the bash 3.x weirdness.

pda added 3 commits June 27, 2020 15:19
mapfile was introduced in bash 4.x
macOS ships with bash 3.x
Take advice from shellcheck: https://github.com/koalaman/shellcheck/wiki/SC2207
bash 3.x treats ${foo[*]} on an empty array as an unbound variable.
Newer bash 4.x and 5.x does not have this problem.

We can use ${#foo[@]} instead, to check if the array is empty.
@pda pda force-pushed the bash-3-avoid-mapfile branch from e2ed9bc to 5ac470d Compare June 27, 2020 05:19
@pda
Copy link
Contributor Author

pda commented Jun 27, 2020

(also, rebased onto #49 to get rid of the already-failing acceptance test; should give us a ✅ on this PR)

@pda pda merged commit 06be3a0 into master Jun 29, 2020
@pda pda deleted the bash-3-avoid-mapfile branch June 29, 2020 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of mapfile introduces bash4 dependency making local runs difficult on macOS
5 participants