-
Notifications
You must be signed in to change notification settings - Fork 25
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
Initialise bash-lib repo #1
Conversation
Review notes:
|
ebdb2bc
to
c5510a2
Compare
c5510a2
to
d8b85ec
Compare
This looks great @hughsaunders! I didn't realize (my ignorance) there were decent tools for testing Shell scripts. |
b267815
to
4eabe3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hughsaunders Left some comments - most were style based though
git-subtree-dir: test-utils/bats git-subtree-split: 2e3bac14d4201481d270310317e00d116d4355be
git-subtree-dir: test-utils/bats-assert-1 git-subtree-split: 08c40b485c08f82eb17e4d6e1ba052eee18cabd5
git-subtree-dir: test-utils/bats-support git-subtree-split: 64e7436962affbe15974d181173c37e1fac70073
552cf9a
to
5f1db2c
Compare
Notes for review round 2
15:31 $ git diff --stat review_round_1 review_round_2_no_subtrees
.gitmodules | 9 ----
.gittrees | 13 +++++
Jenkinsfile | 18 +++----
README.md | 135 +++++++++++++++++++++++-----------------------
filehandling/{lib.sh => lib} | 4 +-
git/lib | 79 +++++++++++++++++++++++++++
git/lib.sh | 13 -----
helpers/lib | 20 +++++++
helpers/lib.sh | 18 -------
init.sh => init | 18 +++----
k8s/Dockerfile | 38 ++++++++-----
k8s/{lib.sh => lib} | 16 +++---
k8s/{platform_login.sh => platform_login} | 4 ++
logging/{lib.sh => lib} | 2 +-
run-tests | 16 ++++++
run-tests.sh | 13 -----
test-utils/bats | 1 -
test-utils/bats-assert-1 | 1 -
test-utils/bats-support | 1 -
test-utils/{lib.sh => lib} | 25 +++++----
test-utils/tap2junit/Dockerfile | 9 ++--
test-utils/tap2junit/tap2junit.py | 22 ++++----
tests-for-this-repo/filehandling.bats | 6 +--
tests-for-this-repo/git.bats | 138 ++++++++++++++++++++++++++++++++++++++++++------
tests-for-this-repo/helpers.bats | 12 ++---
tests-for-this-repo/k8s.bats | 6 +--
tests-for-this-repo/lint.bats | 81 +++++++++++++---------------
tests-for-this-repo/logging.bats | 6 +--
tests-for-this-repo/python-lint.sh | 18 -------
tests-for-this-repo/python-lint/Dockerfile | 9 ++--
tests-for-this-repo/{run-bats-tests.sh => run-bats-tests} | 41 ++++++++------
tests-for-this-repo/run-gitleaks | 18 +++++++
tests-for-this-repo/run-python-lint | 20 +++++++
tests-for-this-repo/test-utils.bats | 79 ++++++++++++++-------------
34 files changed, 567 insertions(+), 342 deletions(-) |
cb85f6d
to
19dd0bf
Compare
This commit adds the structure of the libraries and a few funcions. Most of the code in this commit is geared towards making sure this repo stays tested and documented in future. The following things are checked: * All functions are documented * All functions are tested * All scripts are linted See Readme for a list of the functions that are included, and for more information on testing with BATS.
19dd0bf
to
1a441a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hughsaunders Per our convo earlier, approved on the PR and the notes for FIXMEs is below:
Feel strongly about
- Subtrees get out of hand quickly for even the smallest of repos. We should try to find a way to not force all users to deal with ever-growing helper lib checkouts.
filehandling/lib
(even though it was straight from SO) at the very least needs some comments but it really could be better if the logic flow was cleaned up to be more readable.- The
: ${FOO:?"Something"}
pattern is really hard to read when compared to an if/else - The
[[ <check> ]] | die "foo"
and<cmd> || die
patterns are also really hard to read when compared to an if/else - If/else pattern where the else is returning 1 should be changed to be
if !condition return 1
and then do the positive condition (unconditionally) tests-for-this-repo
needs a better name. Don't know what it should be but the current one isn't great.- Bats tests and Dockerfiles should have more liberal use of newlines in tests to show given/when/then sections
[[ <check> ]]
by itself should go away as a pattern as it's unclear what it's doing without knowing bash in depth
Should probably fix
local rc=0
/return $rc
pattern looked in most cases superfluous. Some tests do seem to need it but we should probably use a named variable rather than return code as the saved state variable.- https://github.com/conjurinc/bash-lib/pull/1/files#diff-9e74f9f10627a857b3f795b8264c89adR14 space between
"
and\
- https://github.com/conjurinc/bash-lib/pull/1/files#diff-9e74f9f10627a857b3f795b8264c89adR30 - should probably quote the image/tag variable
- run_tests should probably do a loop over an array rather than have 3 individual sections on what it's testing
- We should probably use
echo
withecho message
instead ofecho -e '\nmessage
- tests-for-this-repo/run-bats-tests quote both sting values that are being compared
On the fence but leaning towards needing a fix
- Some of your awk chomping of refs could be done with sed
- Even though all of the script import
-e
flag, I think it would be good to explicitly set it in all toplevelish scripts that you expect might want to bail on errors - k8s/platform_login seems like it probably doesn't need main function given that the whole thing is tiny
Nits
- https://github.com/conjurinc/bash-lib/pull/1/files#diff-04c6e90faac2675aa89e2176d2eec7d8R124 needs bold tags
- tests-for-this-repo/python-lint/Dockerfile
/mnt
volume would probably be something else
Random thoughts
- https://github.com/conjurinc/bash-lib/pull/1/files#diff-e37f0136aa3ffaf149b351f6a4c948e9R34 We should probably think about what happens if the used doesn't have credentials to to the submodule checkout but they have the code already on the machine.
- I think pylint is a bit better than pyflake but I'd be curious to see how pyflake works nowadays
@dividedmind When you get a chance, can you go through this PR (it's pretty bulky though) and this comment to see what items seem worth addressing in a followup PR from @hughsaunders? |
This commit adds the structure of the libraries and a few funcions.
Most of the code in this commit is geared towards making sure this
repo stays tested and documented in future.
The following things are checked:
* All functions are documented
* All functions are tested
* All scripts are linted
See Readme for a list of the functions that are included, and for more
information on testing with BATS.