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

Initialise bash-lib repo #1

Merged
merged 7 commits into from
May 3, 2019
Merged

Initialise bash-lib repo #1

merged 7 commits into from
May 3, 2019

Conversation

hughsaunders
Copy link
Contributor

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.

@hughsaunders
Copy link
Contributor Author

hughsaunders commented Apr 18, 2019

Review notes:

  • There isn't much output in Jenkins when all the tests pass, check the tests tab to see the list of tests performed.
  • Maybe start with the readme.
  • This is a large PR to an empty repo, I suggest we lean towards LGTM, merge it and iterate. However I'm importing via PR incase anyone has any high level objections.

@jvanderhoof
Copy link

This looks great @hughsaunders! I didn't realize (my ignorance) there were decent tools for testing Shell scripts.

@hughsaunders hughsaunders force-pushed the initial_libs branch 2 times, most recently from b267815 to 4eabe3c Compare April 18, 2019 14:06
sgnn7
sgnn7 previously requested changes Apr 18, 2019
Copy link
Contributor

@sgnn7 sgnn7 left a 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

.gitmodules Outdated Show resolved Hide resolved
Jenkinsfile Outdated Show resolved Hide resolved
Jenkinsfile Outdated Show resolved Hide resolved
Jenkinsfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tests-for-this-repo/test-utils.bats Outdated Show resolved Hide resolved
tests-for-this-repo/test-utils.bats Show resolved Hide resolved
tests-for-this-repo/test-utils.bats Outdated Show resolved Hide resolved
tests-for-this-repo/test-utils.bats Outdated Show resolved Hide resolved
tests-for-this-repo/test-utils.bats Outdated Show resolved Hide resolved
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
@hughsaunders hughsaunders force-pushed the initial_libs branch 2 times, most recently from 552cf9a to 5f1db2c Compare April 25, 2019 12:12
@hughsaunders
Copy link
Contributor Author

hughsaunders commented Apr 25, 2019

Notes for review round 2

  • Please err on the side of LGTM, then we can merge this and iterate.
  • This PR now pulls in three subtrees, that code is entirely upstream and should not be reviewed. To only review the original code, view the only non merge commit, currently: 1a441a3.
  • Subtree related functionality has been pulled, because this was taking too long, and subtree functions don't pass their tests due to an old version of git on the Jenkins executors (git 2.8 required as it includes this commit but 2.7.4 is available). WIP Branch
  • If you have already reviewed and just want to see differences since the first round of reviews, clone and use git locally. The GitHub UI can't show diffs between un-mergeable commits 😞
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(-)

@hughsaunders hughsaunders force-pushed the initial_libs branch 4 times, most recently from cb85f6d to 19dd0bf Compare April 25, 2019 14:33
@hughsaunders hughsaunders requested a review from sgnn7 April 25, 2019 14:45
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.
Copy link
Contributor

@sgnn7 sgnn7 left a 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

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

Random thoughts

@sgnn7 sgnn7 merged commit 3d22ac3 into master May 3, 2019
@sgnn7 sgnn7 deleted the initial_libs branch May 3, 2019 17:22
@sgnn7
Copy link
Contributor

sgnn7 commented Jun 19, 2019

@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?

@izgeri izgeri mentioned this pull request Jul 3, 2019
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.

None yet

3 participants