Portability additions #11

housni · 2018-10-26T02:41:36Z

Replaces #!/bin/bash with #!/usr/bin/env bash.
- References:
1. https://en.wikipedia.org/wiki/Shebang_(Unix)#Portability
2. https://en.wikipedia.org/wiki/Shebang_(Unix)#Portability
Makes scripts shellcheck compatible.
Runs shellcheck in CircleCI as part of the CI.
Set pipefail, nounset, errtrace, functrace, etc. via bootstrap.sh to encourage good shell scripting practice.
- References:
1. https://www.davidpashley.com/articles/writing-robust-shell-scripts/

See: #8

brikis98

This looks great, thank you!

It's labeled as WIP... Are there other changes in progress?

README.md

modules/bash-commons/install.sh

modules/bash-commons/src/assert.sh

Co-Authored-By: housni <housni.yakoob@gmail.com>

…sni/bash-commons into feature/portability-additions

housni · 2018-10-27T17:19:20Z

Regarding the WIP label, yes, I wanted to add one more thing with regard to this: #8 (comment)

…commons now. Using custom Dockerfile

housni · 2018-11-12T01:00:33Z

Dockerfile

@@ -0,0 +1,30 @@
+FROM bash:3.2
+
+# TODO: labels here. See: http://label-schema.org/rc1/


@brikis98 , feel free to add to the labels here.

brikis98

Looking great! A few minor comments and this is ready to merge.

.circleci/shellcheck.sh

Dockerfile

README.md

brikis98 · 2018-11-12T15:03:25Z

.circleci/Dockerfile

@@ -1,24 +0,0 @@
-FROM ubuntu:16.04


Why delete this?

This repo needs to be Bash 3 compatible, right? the ubuntu:16.04 image has Bash 4.x while the bash:3.2 image is for Bash 3.x only. This will ensure our tests are more thorough and we can guarantee that the code will work in 3.x.

I was actually more curious about why delete the whole Dockerfile rather than just the ubuntu portion. The original Dockerfile installs lots of utilities we need at test time, including the AWS CLI, flask, moto, iptables, etc. It doesn't look like those are part of the new Dockerfile. I'd expect this to cause many of the AWS tests to fail...

Also, I'm in favor of testing with an older version of bash to ensure broader compatibility, but it looks like the bash:3.2 image might be running on top of Alpine linux (judging by the use of apk), which is lovely in terms of size, but, in my experience, awful in terms of usability; the libraries it exposes aren't nearly as comprehensive or up to date as APT, and you end up having to do a lot more work to install basic dependencies.

Oh, I deleted the Dockerfile because it wasn't used. I also cross-referenced it with the history of your gruntwork/bash-commons-circleci-tests image and found some inconsistencies so I assumed it was a residual file. Shall I bring it back?

Just FYI, all the tests pass. I make sure they pass before I commit.
A docker-compose up will run both shellcheck and bats.

The bats tests still use your gruntwork/bash-commons-circleci-tests image. The Dockerfile is only used for the shellcheck test.

Here's a full test I just ran:

$ docker-compose up Starting bashcommons_shellcheck_1 ... Starting bashcommons_shellcheck_1 Starting bashcommons_bats_1 ... Starting bashcommons_bats_1 ... done Attaching to bashcommons_shellcheck_1, bashcommons_bats_1 shellcheck_1 | ShellCheck - shell script analysis tool shellcheck_1 | version: 0.5.0 shellcheck_1 | license: GNU General Public License, version 3 shellcheck_1 | website: https://www.shellcheck.net shellcheck_1 | shellcheck_1 | Shellcheck is scanning some files (12): shellcheck_1 | /usr/local/src/bash-commons/.circleci/shellcheck.sh shellcheck_1 | /usr/local/src/bash-commons/.circleci/aws-local.sh shellcheck_1 | /usr/local/src/bash-commons/modules/bash-commons/src/array.sh shellcheck_1 | /usr/local/src/bash-commons/modules/bash-commons/src/aws-wrapper.sh shellcheck_1 | /usr/local/src/bash-commons/modules/bash-commons/src/os.sh shellcheck_1 | /usr/local/src/bash-commons/modules/bash-commons/src/string.sh shellcheck_1 | /usr/local/src/bash-commons/modules/bash-commons/src/bootstrap.sh shellcheck_1 | /usr/local/src/bash-commons/modules/bash-commons/src/assert.sh shellcheck_1 | /usr/local/src/bash-commons/modules/bash-commons/src/file.sh shellcheck_1 | /usr/local/src/bash-commons/modules/bash-commons/src/aws.sh shellcheck_1 | /usr/local/src/bash-commons/modules/bash-commons/src/log.sh shellcheck_1 | /usr/local/src/bash-commons/modules/bash-commons/install.sh shellcheck_1 | bats_1 | 1..144 bats_1 | ok 1 array_contains on empty array bats_1 | ok 2 array_contains on array of length 1 for non matching item bats_1 | ok 3 array_contains on array of length 1 for matching item bats_1 | ok 4 array_contains on array of length 3 for non matching item bats_1 | ok 5 array_contains on array of length 3 for matching item bats_1 | ok 6 array_contains on array of length 3 for multiple matches bats_1 | ok 7 array_contains on array of length 3 with spaces in array values bats_1 | ok 8 array_join on empty array bats_1 | ok 9 array_join on array of length 1 bats_1 | ok 10 array_join on array of length 3 bats_1 | ok 11 array_join on array of length 3 with multi character separator shellcheck_1 | All files successfully scanned with no issues bats_1 | ok 12 assert_is_installed on bash built in bashcommons_shellcheck_1 exited with code 0 bats_1 | ok 13 assert_is_installed on installed app bats_1 | ok 14 assert_is_installed on non-existent app bats_1 | ok 15 assert_not_empty on empty string bats_1 | ok 16 assert_not_empty on non-empty string bats_1 | ok 17 assert_not_empty on null string bats_1 | ok 18 assert_not_empty on empty array bats_1 | ok 19 assert_not_empty on non-empty array bats_1 | ok 20 assert_empty on empty string bats_1 | ok 21 assert_empty on non-empty string bats_1 | ok 22 assert_empty on empty array bats_1 | ok 23 assert_empty on non-empty array bats_1 | ok 24 assert_not_empty_or_null on empty string bats_1 | ok 25 assert_not_empty_or_null on non-empty string bats_1 | ok 26 assert_not_empty_or_null on null string bats_1 | ok 27 assert_not_empty_or_null on empty array bats_1 | ok 28 assert_not_empty_or_null on non-empty array bats_1 | ok 29 assert_value_in_list empty list bats_1 | ok 30 assert_value_in_list list of length 1, no match bats_1 | ok 31 assert_value_in_list list of length 3, no match bats_1 | ok 32 assert_value_in_list list of length 1, match bats_1 | ok 33 assert_value_in_list list of length 3, match bats_1 | ok 34 assert_value_in_list list of length 3, with spaces in the values, no match bats_1 | ok 35 assert_uid_is_root_or_sudo as root bats_1 | ok 36 aws_get_instance_tags empty bats_1 | ok 37 aws_get_instance_tags non-empty bats_1 | ok 38 aws_describe_asg empty bats_1 | ok 39 aws_describe_asg non-empty bats_1 | ok 40 aws_describe_instances_in_asg empty bats_1 | ok 41 aws_describe_instances_in_asg non-empty bats_1 | ok 42 aws_get_instances_with_tag empty bats_1 | ok 43 aws_get_instances_with_tag non-empty bats_1 | ok 44 aws_get_instance_private_ip bats_1 | ok 45 aws_get_instance_public_ip bats_1 | ok 46 aws_get_instance_private_hostname bats_1 | ok 47 aws_get_instance_public_hostname bats_1 | ok 48 aws_get_instance_id bats_1 | ok 49 aws_get_instance_region bats_1 | ok 50 aws_get_ec2_instance_availability_zone bats_1 | ok 51 aws_wrapper_get_asg_name ASG tag only bats_1 | ok 52 aws_wrapper_get_asg_name ASG tag and other tags bats_1 | ok 53 aws_wrapper_get_asg_name no tags bats_1 | ok 54 aws_wrapper_get_asg_name no ASG tags bats_1 | ok 55 aws_wrapper_get_instance_tag no tags bats_1 | ok 56 aws_wrapper_get_instance_tag no matching tag bats_1 | ok 57 aws_wrapper_get_instance_tag matching tag bats_1 | ok 58 aws_wrapper_wait_for_instance_tags one tag bats_1 | ok 59 aws_wrapper_wait_for_instance_tags multiple tags bats_1 | ok 60 aws_wrapper_wait_for_instance_tags no tags bats_1 | ok 61 aws_wrapper_wait_for_instance_tags no tags bats_1 | ok 62 aws_wrapper_get_asg_size ASG exists bats_1 | ok 63 aws_wrapper_get_asg_size ASG does not exist bats_1 | ok 64 aws_wrapper_wait_for_instances_in_asg ASG size 1 bats_1 | ok 65 aws_wrapper_wait_for_instances_in_asg ASG size 3 bats_1 | ok 66 aws_wrapper_wait_for_instances_in_asg ASG size 2, 3 available bats_1 | ok 67 aws_wrapper_wait_for_instances_in_asg ASG size 3, only 2 available bats_1 | ok 68 aws_wrapper_wait_for_instances_in_asg ASG size not available bats_1 | ok 69 aws_wrapper_get_ips_in_asg empty ASG bats_1 | ok 70 aws_wrapper_get_ips_in_asg ASG size 1, public IPs bats_1 | ok 71 aws_wrapper_get_ips_in_asg ASG size 3, public IPs bats_1 | ok 72 aws_wrapper_get_ips_in_asg ASG size 3, private IPs bats_1 | ok 73 aws_wrapper_get_hostnames_in_asg empty ASG bats_1 | ok 74 aws_wrapper_get_hostnames_in_asg ASG size 1, public IPs bats_1 | ok 75 aws_wrapper_get_hostnames_in_asg ASG size 3, public IPs bats_1 | ok 76 aws_wrapper_get_hostnames_in_asg ASG size 3, private IPs bats_1 | ok 77 aws_wrapper_get_hostname public bats_1 | ok 78 aws_wrapper_get_hostname private bats_1 | ok 79 file_exists on non-existent file bats_1 | ok 80 file_exists on real file bats_1 | ok 81 file_contains_text on non-existent file bats_1 | ok 82 file_contains_text no match bats_1 | ok 83 file_contains_text match bats_1 | ok 84 file_contains_text regex match bats_1 | ok 85 file_append_text once bats_1 | ok 86 file_append_text multiple times bats_1 | ok 87 file_replace_text empty file bats_1 | ok 88 file_replace_text non empty file, no match bats_1 | ok 89 file_replace_text non empty file, exact match bats_1 | ok 90 file_replace_text non empty file, regex match bats_1 | ok 91 file_replace_or_append_text empty file bats_1 | ok 92 file_replace_or_append_text non empty file, no match bats_1 | ok 93 file_replace_or_append_text non empty file, exact match bats_1 | ok 94 file_replace_or_append_text non empty file, regex match one line bats_1 | ok 95 log_info string bats_1 | ok 96 log_warn string bats_1 | ok 97 log_error string bats_1 | ok 98 os_get_available_memory_mb bats_1 | ok 99 os_is_amazon_linux bats_1 | ok 100 os_is_ubuntu bats_1 | ok 101 os_is_darwin bats_1 | ok 102 os_validate_checksum valid sha256 bats_1 | ok 103 os_validate_checksum invalid sha256 bats_1 | ok 104 os_validate_checksum valid md5 bats_1 | ok 105 os_validate_checksum invalid md5 bats_1 | ok 106 os_command_is_installed bash built in bats_1 | ok 107 os_command_is_installed installed app bats_1 | ok 108 os_command_is_installed non-existent app bats_1 | ok 109 os_get_current_users_name bats_1 | ok 110 os_get_current_users_group bats_1 | ok 111 os_user_is_root_or_sudo for root user bats_1 | ok 112 string_contains haystack empty needle empty, match bats_1 | ok 113 string_contains haystack empty needle has value, no match bats_1 | ok 114 string_contains haystack has value, needle empty, match bats_1 | ok 115 string_contains haystack has value, needle has value, no match bats_1 | ok 116 string_contains haystack has value, needle has value, exact match bats_1 | ok 117 string_contains haystack has value, needle has value, internal match bats_1 | ok 118 string_multiline_contains haystack empty needle empty, match bats_1 | ok 119 string_multiline_contains haystack empty needle has value, no match bats_1 | ok 120 string_multiline_contains haystack has value, needle empty, match bats_1 | ok 121 string_multiline_contains haystack has value, needle has value, no match bats_1 | ok 122 string_multiline_contains haystack has value, needle has value, exact match bats_1 | ok 123 string_multiline_contains haystack has value, needle has value, internal match bats_1 | ok 124 string_multiline_contains haystack has multiline value, needle has value, internal match bats_1 | ok 125 string_multiline_contains haystack has multiline value, needle has value, no match bats_1 | ok 126 string_multiline_contains haystack has multiline value, needle has value, regex match bats_1 | ok 127 string_to_uppercase input lowercase bats_1 | ok 128 string_to_uppercase input uppercase bats_1 | ok 129 string_to_uppercase input mixed bats_1 | ok 130 string_strip_prefix empty string, empty prefix bats_1 | ok 131 string_strip_prefix empty string, non empty prefix bats_1 | ok 132 string_strip_prefix non empty string, empty prefix bats_1 | ok 133 string_strip_prefix non empty string, non empty prefix, no match bats_1 | ok 134 string_strip_prefix non empty string, non empty prefix, exact match bats_1 | ok 135 string_strip_prefix non empty string, non empty prefix, wildcard match bats_1 | ok 136 string_strip_suffix empty string, empty suffix bats_1 | ok 137 string_strip_suffix empty string, non empty suffix bats_1 | ok 138 string_strip_suffix non empty string, empty suffix bats_1 | ok 139 string_strip_suffix non empty string, non empty suffix, no match bats_1 | ok 140 string_strip_suffix non empty string, non empty suffix, exact match bats_1 | ok 141 string_strip_suffix non empty string, non empty suffix, wildcard match bats_1 | ok 142 string_is_empty_or_null empty string bats_1 | ok 143 string_is_empty_or_null null string bats_1 | ok 144 string_is_empty_or_null non empty string bashcommons_bats_1 exited with code 0

At least part of the tests are running in the Docker image gruntwork/bash-commons-circleci-tests. I believe this is the Dockerfile for that Docker image. If we delete it, we won't be able to build new images!

That's probably not obvious, so consider updating the docker-compose.yml to use build instead of image and point at this file...

I can certainly put it back if you want, no problem! But before that, I wanted to bring to your attention that the image doesn't seem to be the same as the original Dockerfile but I could be mistaken...but here's what I see:

$ docker history -H --no-trunc=true gruntwork/bash-commons-circleci-tests IMAGE CREATED CREATED BY SIZE COMMENT sha256:fa287703f1bb559a314de56bf3a41040bc4e6c9075a7ff5a608a15cc9ed5befb 7 months ago /bin/sh -c #(nop) COPY file:1dcda1edf7f2ca4e46af9e796447ed81a7b8da1dfb6ea016af783d4961d16108 in /usr/local/bin/aws 659B <missing> 7 months ago /bin/sh -c apt-get install -y net-tools iptables 3.5MB <missing> 7 months ago /bin/sh -c pip install flask moto moto[server] 46.2MB <missing> 7 months ago /bin/sh -c pip install awscli --upgrade --user 46.8MB <missing> 7 months ago /bin/sh -c apt-get install -y software-properties-common && add-apt-repository ppa:duggan/bats && DEBIAN_FRONTEND=noninteractive apt-get update && apt-get install -y bats 84.6MB <missing> 7 months ago /bin/sh -c DEBIAN_FRONTEND=noninteractive apt-get update && apt-get install -y vim python-pip jq sudo curl 367MB <missing> 7 months ago /bin/sh -c #(nop) MAINTAINER Gruntwork <info@gruntwork.io> 0B <missing> 9 months ago /bin/sh -c #(nop) CMD ["/bin/bash"] 0B <missing> 9 months ago /bin/sh -c mkdir -p /run/systemd && echo 'docker' > /run/systemd/container 7B <missing> 9 months ago /bin/sh -c sed -i 's/^#\s*$deb.*universe$$/\1/g' /etc/apt/sources.list 2.76kB <missing> 9 months ago /bin/sh -c rm -rf /var/lib/apt/lists/* 0B <missing> 9 months ago /bin/sh -c set -xe && echo '#!/bin/sh' > /usr/sbin/policy-rc.d && echo 'exit 101' >> /usr/sbin/policy-rc.d && chmod +x /usr/sbin/policy-rc.d && dpkg-divert --local --rename --add /sbin/initctl && cp -a /usr/sbin/policy-rc.d /sbin/initctl && sed -i 's/^exit.*/exit 0/' /sbin/initctl && echo 'force-unsafe-io' > /etc/dpkg/dpkg.cfg.d/docker-apt-speedup && echo 'DPkg::Post-Invoke { "rm -f /var/cache/apt/archives/*.deb /var/cache/apt/archives/partial/*.deb /var/cache/apt/*.bin || true"; };' > /etc/apt/apt.conf.d/docker-clean && echo 'APT::Update::Post-Invoke { "rm -f /var/cache/apt/archives/*.deb /var/cache/apt/archives/partial/*.deb /var/cache/apt/*.bin || true"; };' >> /etc/apt/apt.conf.d/docker-clean && echo 'Dir::Cache::pkgcache ""; Dir::Cache::srcpkgcache "";' >> /etc/apt/apt.conf.d/docker-clean && echo 'Acquire::Languages "none";' > /etc/apt/apt.conf.d/docker-no-languages && echo 'Acquire::GzipIndexes "true"; Acquire::CompressionTypes::Order:: "gz";' > /etc/apt/apt.conf.d/docker-gzip-indexes && echo 'Apt::AutoRemove::SuggestsImportant "false";' > /etc/apt/apt.conf.d/docker-autoremove-suggests 745B <missing> 9 months ago /bin/sh -c #(nop) ADD file:a3344b835ea6fdc5692df23826c970403656c6947342e117b2ac1a05956679af in / 112MB

And this is the original Dockerfile: https://github.com/gruntwork-io/bash-commons/blob/master/.circleci/Dockerfile

They seem identical for the most part but there are some very minor inconsistencies. Would you rather I just restore this Dockerfile or can we place the actual Dockerfile here instead?

It looks 99% identical, so I'd put it back.

Done. I ran the tests via Docker Compose which now builds the image off Dockerfiles and, as Anakin Skywalker would say, it's working, it's working!

modules/bash-commons/src/assert.sh

brikis98 · 2018-11-12T15:05:25Z

.circleci/shellcheck.sh

+# Any trap on ERR is inherited by shell functions, command substitutions, and
+# commands executed in a subshell environment. The ERR trap is normally not
+# inherited in such cases.
+set -o errtrace


Why are all these copy/pasted into this script instead of using bootstrap.sh?

I was going to just source bootstrap.sh...but since this is a CI script meant to check our library, I figured it would be better if potential future mistakes in bootstrap.sh don't break our CI. What do you think about that?

It's a trade-off. On the one hand, you're right, it's a bit awkward to mix the code we're testing with the code we're using to test it. On the other hand, every time we update bootstrap.sh, we'll end up either (a) updating shellcheck.sh too, in which case, it seems pointless to keep it separate or (b) we'll forget to update shellcheck.sh, in which case, it may end up causing bugs in and of itself.

Seems fair to me. I'll source bootstrap.sh from shellcheck.sh.

The location of bootstrap.sh bothers me. It seems like it shouldn't be there, mixed with the other modules but we'll either have to change structure of how the modules are organized or the installation instructions.

housni · 2018-11-12T20:30:07Z

Wow, I made so many mistakes! That's what happens when you code at 4am on a Monday morning :D
Thanks for the awesome PR review! If you find more mistakes, please let me know.

brikis98

This is fantastic, thank you! Merging now and will let tests run. If they pass, I'll issue a new release.

brikis98 · 2018-11-28T12:44:35Z

https://github.com/gruntwork-io/bash-commons/releases/tag/v0.1.0

Changed shebang to be more portable.

bd0ec4e

housni changed the title ~~Portability additions~~ WIP: Portability additions Oct 26, 2018

housni added 2 commits October 26, 2018 05:53

Made scripts shellcheck compatible

4453b38

Updated docs to match previous commits

717a3d9

housni mentioned this pull request Oct 26, 2018

I'd like to submit a PR, but first... #8

Closed

brikis98 reviewed Oct 27, 2018

View reviewed changes

README.md Show resolved Hide resolved

modules/bash-commons/install.sh Show resolved Hide resolved

modules/bash-commons/src/assert.sh Outdated Show resolved Hide resolved

modules/bash-commons/src/assert.sh Outdated Show resolved Hide resolved

brikis98 and others added 3 commits October 27, 2018 16:31

Added back note about readonly variables.

999bd45

Co-Authored-By: housni <housni.yakoob@gmail.com>

'list' should also be an array

0f9ec24

Merge branch 'feature/portability-additions' of github-housni.com:hou…

871d7a7

…sni/bash-commons into feature/portability-additions

housni added 6 commits November 5, 2018 04:53

Moved docker-compose.yml to root and removed unused Dockerfile

12e1f23

Changed shebang to be more portable.

4d90070

Integrated shellcheck into CI.

4751c7a

Shellcheck runs from root so source paths should be relative to root.

9d391b4

Fixed mount path

a7e9f4b

Added bootstrap.sh along with docs for it

97ca6f9

housni changed the title ~~WIP: Portability additions~~ Portability additions Nov 5, 2018

housni added 3 commits November 12, 2018 03:54

Removing set -e since that option should be handled by bootstrap.sh

e097be8

Separated bats and shellcheck tests. Mounting to /usr/local/src/bash-…

21fe73a

…commons now. Using custom Dockerfile

CircleCI now confirms to Docker Compose

5cdb8bb

housni commented Nov 12, 2018

View reviewed changes

brikis98 reviewed Nov 12, 2018

View reviewed changes

housni added 2 commits November 12, 2018 23:27

Better code and quoting variables

1c9d8d6

Reverting previous change. Using @ here is valid in this context.

cf1efa8

housni added 5 commits November 13, 2018 03:32

Removed less eager splitting via IFS on the request of @brikis98

855e325

Grammar fix

29e7542

Using bootstrap.sh to set some options

7053dc7

Using local Dockerfiles for all tests

a151c11

Merge branch 'master' into feature/portability-additions

5e22924

Remove reference to old image

108dd13

brikis98 approved these changes Nov 28, 2018

View reviewed changes

brikis98 merged commit 9369cb3 into gruntwork-io:master Nov 28, 2018

brikis98 mentioned this pull request Oct 2, 2020

Local readonly variables not declared correctly #9

Closed

		@@ -0,0 +1,30 @@
		FROM bash:3.2

		# TODO: labels here. See: http://label-schema.org/rc1/

Uh oh!

Portability additions #11

Portability additions #11

Uh oh!

Conversation

housni commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brikis98 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

housni commented Oct 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brikis98 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

housni commented Nov 12, 2018

Uh oh!

brikis98 left a comment

Choose a reason for hiding this comment

Uh oh!

brikis98 commented Nov 28, 2018

Uh oh!

Uh oh!

housni commented Oct 26, 2018 •

edited

Loading