Skip to content

New pre commit hook - #3

Open
rajk1000 wants to merge 43 commits into
mainfrom
new_pre_commit_hook
Open

rajk1000 wants to merge 43 commits into
mainfrom
new_pre_commit_hook

Conversation

@rajk1000

@rajk1000 rajk1000 commented Sep 8, 2022

Copy link
Copy Markdown
Owner

No description provided.

@pspandler pspandler left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just cosmetic stuff and standardisation needed for now - fixing this and adding comments will help.

Comment thread .githooks/pre-commit Outdated
#!/bin/bash
#set -x
PREFIX="pre-commit:"
RESULTS_FILE=tfsec-scan-output.txt

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Quote your variables please.

Also, it would help if there were some useful comments - reading comments aids understanding.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ok

Comment thread .githooks/pre-commit Outdated
initialise() {
mkdir -p $TFSEC_WORKING_DIR
mkdir -p $TFSEC_RESULTS_DIR
rm $TFSEC_WORKING_DIR/* 2> /dev/null

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove stray space.

Why are you redirecting STDERR to /dev/null?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ok. Redirecting as it gives an error if this is run for the first time

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Righto, maybe you could:

  • see if the directory exists
  • if yes, delete it & recreate it
  • if no, create it

Comment thread .githooks/pre-commit Outdated
#set -x
PREFIX="pre-commit:"
RESULTS_FILE=tfsec-scan-output.txt
TFSEC_RESULTS_DIR=/tmp/tfsec-results

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ditto.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ok

Comment thread .githooks/pre-commit Outdated
PREFIX="pre-commit:"
RESULTS_FILE=tfsec-scan-output.txt
TFSEC_RESULTS_DIR=/tmp/tfsec-results
TFSEC_WORKING_DIR=/tmp/tfsec-files

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ditto.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ok

Comment thread .githooks/pre-commit Outdated
mkdir -p $TFSEC_WORKING_DIR
mkdir -p $TFSEC_RESULTS_DIR
rm $TFSEC_WORKING_DIR/* 2> /dev/null
rm $TFSEC_RESULTS_DIR/* 2> /dev/null

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ditto.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ok

Comment thread .githooks/pre-commit Outdated
initialise;
populateGlobalVars;

if [ "$TERRAFORM_FILES_STAGED" = "yes" ]; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Are you using [ ] or [[ ]]..?

I think [[ ]] is safer, but I can't remember why.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

[[ now

Comment thread .githooks/pre-commit Outdated
copyTerraformFilesToBeScanned;

#Do the latest tfsec scan on the files that have been staged
if ! tfsec $TFSEC_WORKING_DIR --no-colour --out $TFSEC_RESULTS_DIR/$RESULTS_FILE; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Quoting and cuddling, blah, blah.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ok

Comment thread .githooks/pre-commit Outdated
if [ "$ALLOW_TFSEC_ISSUES" = "yes" ]
then
#Check that the file added is exactly the same as the latest tfsec issues. They must be identical in order for the commit to be allowed through
diffResult=$(diff $TFSEC_RESULTS_DIR/$RESULTS_FILE $TFSEC_WORKING_DIR/$RESULTS_FILE)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ditto.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ok

Comment thread .githooks/pre-commit Outdated
diffResult=$(diff $TFSEC_RESULTS_DIR/$RESULTS_FILE $TFSEC_WORKING_DIR/$RESULTS_FILE)
if [ $? -eq 0 ]
then
informUserThatWeAllowCommitThroughDespiteTfsecIssues;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not a fan of Camel case function names.

Also, could they be shorter?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ok

Comment thread .githooks/pre-commit Outdated
exit 1
fi
else
informUserThatWeDenyCommitBecauseOutstandingTfSecIssues;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Indentation

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ok

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.

2 participants