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

pre-commit pass through #1844

Merged
merged 11 commits into from Mar 14, 2019

Conversation

3 participants
@rdhariwal
Copy link
Contributor

rdhariwal commented Mar 8, 2019

Description

The goal of this PR is to do a pass through all hooks that fire as part of pre-commit.

Reviewer Notes

Tracked issues

  1. Fix for go-lint not running against project files
    image
  2. Enable swagger validation on dps endpoint

Is there anything you would like reviewers to give additional scrutiny?

Setup

  1. Stage a broken go file, see my screenshot above for example
  2. run pre-commit and go-vet and go-lint hooks should fail with errors
  3. Stage a broken change in swagger/dps.yaml
  4. run pre-commit or pre-commit run swagger
  5. DPS Swagger hook should fail
  6. feel free to test any other hook by staging broken file

Code Review Verification Steps

  • Have been communicated to #dp3-engineering
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 8, 2019

Codecov Report

Merging #1844 into master will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1844      +/-   ##
==========================================
+ Coverage   49.56%   49.65%   +0.08%     
==========================================
  Files         429      428       -1     
  Lines       18479    18448      -31     
  Branches     1632     1632              
==========================================
  Hits         9160     9160              
+ Misses       8515     8484      -31     
  Partials      804      804

@rdhariwal rdhariwal force-pushed the rd-pre-commit-cleanup-pass branch from 96e9c43 to 2fe947c Mar 8, 2019

@rdhariwal rdhariwal requested a review from chrisgilmerproj Mar 13, 2019

@rdhariwal

This comment has been minimized.

Copy link
Contributor Author

rdhariwal commented Mar 13, 2019

still wrapping up documentation changes :)

@rdhariwal rdhariwal requested review from stangah and pjdufour-truss Mar 13, 2019

@@ -0,0 +1,31 @@
# Run and troubleshoot pre-commit hook's

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Mar 13, 2019

Contributor

Just hooks

@rdhariwal rdhariwal force-pushed the rd-pre-commit-cleanup-pass branch from f837280 to 1a2e6ca Mar 13, 2019

@rdhariwal rdhariwal changed the title WIP: pre-commit pass through pre-commit pass through Mar 13, 2019

@rdhariwal rdhariwal removed the wip label Mar 13, 2019


exec 5>&1
for file in "$@"; do
output="$(golint "$file" 2>&1 | tee /dev/fd/5)"

This comment has been minimized.

@rdhariwal

rdhariwal Mar 13, 2019

Author Contributor

@chrisgilmerproj so the new golint hook will be invoked through a local script.. ^^ this one to be precise. Should I be adding golint to Gopkg.toml to make sure it is installed

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 13, 2019

Contributor

I think this may have fixed it: https://defensedigitalservice.slack.com/archives/G8P7UD05U/p1552502343254100?thread_ts=1552499777.247400&cid=G8P7UD05U

As long as make server_deps installs golint then we're set.

Pre-commit can be run by simply running the following command in terminal:
`pre-commit`

If you would like to run an individual hook, for example if you want to only run *prettier*: `pre-commit run prettier`

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 13, 2019

Contributor

Consider using -a in all of these. People tend to copy/paste from docs and I'd rather they run against all files then miss some.

[Pre-commit](https://pre-commit.com/) is a powerful tool that automates validations, lint checks and adds to developer quality of life. The config file that determines the actions of pre-commit hook can be found [here](/path/.pre-commit-config.yaml)

Pre-commit can be run by simply running the following command in terminal:
`pre-commit`

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 13, 2019

Contributor

You can also run all of them with make pre_commit_tests which is what CircleCI uses`. I'd at least mention it.

@@ -0,0 +1,31 @@
# Run and troubleshoot pre-commit hooks

[Pre-commit](https://pre-commit.com/) is a powerful tool that automates validations, lint checks and adds to developer quality of life. The config file that determines the actions of pre-commit hook can be found [here](/path/.pre-commit-config.yaml)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 13, 2019

Contributor

Could be worth mentioning how to install pre-commit here. We have make ensure_pre_commit to do the work for you and it should be in our setup guide somewhere.

This comment has been minimized.

@rdhariwal

rdhariwal Mar 14, 2019

Author Contributor

added a link to prerequisites so we don't dupe instructions

hooks:
# - id: go-vet uncomment when go@1.12 is supported for this hook

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 13, 2019

Contributor

Let's get this back even if we have to write another bin/ script like you have for golint`.

This comment has been minimized.

@rdhariwal

rdhariwal Mar 14, 2019

Author Contributor

Sounds good, i tested out go lint again with the latest release from dnephin/pre-commit-golang#25

go vet worked but go lint still passes for us when it should be failing.

So I plan on bringing the go vet command as a local script.

| gosec | Inspects source code for security problems by scanning the Go AST. For more see [here](https://github.com/securego/gosec)
| gen-docs |Attempts to generate table of contents for the [docs/README](docs/README.md) file in doc folder |
| dep-version | checks the dep version |
| dep-check | Runs the command `dep check` to ensure dependencies are in sync with the `Gopkg.toml` file |

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 13, 2019

Contributor

This is fantastic documentation!

@rdhariwal

This comment has been minimized.

Copy link
Contributor Author

rdhariwal commented Mar 14, 2019

image
I added go vet back 😭

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

chrisgilmerproj commented Mar 14, 2019

Like we discussed online, I agree we could probably move go vet to another branch.

# entry: bin/pre-commit-go-vet
# language: script
# files: \.go$
# exclude: vendor/

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 14, 2019

Contributor

👍 - thanks for having this here.

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

Nice!

Can any of this be applied to the ppp-infra repo as well?

@rdhariwal

This comment has been minimized.

Copy link
Contributor Author

rdhariwal commented Mar 14, 2019

Nice!

Can any of this be applied to the ppp-infra repo as well?

not sure atm, but I ll do a pass through. Should be faster now that I understand pre-commit a lot better. I ll play the pp story after go vet makes it in as well, already started that story :)

@rdhariwal rdhariwal merged commit 8eee981 into master Mar 14, 2019

19 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: client_test_coverage Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing fbaec44...cf99f14
Details
codecov/project 49.65% (+0.08%) compared to fbaec44
Details

@rdhariwal rdhariwal deleted the rd-pre-commit-cleanup-pass branch Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.