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

Middleware Refactor #2077

Merged
merged 2 commits into from May 6, 2019

Conversation

2 participants
@pjdufour-truss
Copy link
Contributor

commented May 2, 2019

Description

This PR refactors our middleware into a package (github.com/transcom/mymove/pkg/middleware). This PR also uses the new cli pkg as well.

Reviewer Notes

  • Take a look at the unit tests (You can just run as go test -v to see output).
  • Any regressions?

Setup

None

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • The requirements listed in
    Querying the Database Safely
    have been satisfied.
  • Any new migrations/schema changes:
    • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
    • Have been communicated to #dp3-engineering
    • Secure migrations have been tested using scripts/run-prod-migrations
  • There are no aXe warnings for UI.
  • This works in Supported Browsers and their phone views (Chrome, Firefox, IE, Edge).
  • Any new client dependencies (Google Analytics, hosted libraries, CDNs, etc) have been:
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • User facing changes have been reviewed by design.
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

None

Screenshots

N.A.

@pjdufour-truss pjdufour-truss requested a review from chrisgilmerproj May 2, 2019


// CheckMiddleWare validates middleware command line flags
func CheckMiddleWare(v *viper.Viper) error {
if maxBodySize := v.GetInt64(MaxBodySizeFlag); maxBodySize < int64(0) {

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 2, 2019

Contributor

Should we put a maximum limit on this?

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss May 2, 2019

Author Contributor

That doesn't feel necessary.


// InitMiddlewareFlags initializes the Middleware command line flags
func InitMiddlewareFlags(flag *pflag.FlagSet) {
flag.Int64(MaxBodySizeFlag, MaxBodySizeDefault, "The maximum request body size in bytes as int")

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 2, 2019

Contributor

I love that this is now a flag!

@chrisgilmerproj
Copy link
Contributor

left a comment

I had one question about an upper limit but otherwise this is good to go once it passes tests!

@codecov

This comment has been minimized.

Copy link

commented May 2, 2019

Codecov Report

Merging #2077 into master will increase coverage by 0.12%.
The diff coverage is 90.38%.

@@            Coverage Diff             @@
##           master    #2077      +/-   ##
==========================================
+ Coverage   60.15%   60.27%   +0.12%     
==========================================
  Files         222      228       +6     
  Lines       13248    13299      +51     
==========================================
+ Hits         7969     8015      +46     
- Misses       4310     4313       +3     
- Partials      969      971       +2

@pjdufour-truss pjdufour-truss changed the title [WIP] Middleware Refactor Middleware Refactor May 2, 2019

@pjdufour-truss pjdufour-truss requested review from rdhariwal and sarboc May 2, 2019

pjdufour-truss added some commits May 2, 2019

@pjdufour-truss pjdufour-truss force-pushed the middleware_refactor branch 2 times, most recently from cb6bb27 to eaa0dd3 May 6, 2019

@pjdufour-truss pjdufour-truss merged commit 3c307a2 into master May 6, 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: integration_tests_api 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 90.38% of diff hit (target 60.15%)
Details
codecov/project/go 60.1% (+0.12%) compared to 7cee8a1
Details

@pjdufour-truss pjdufour-truss deleted the middleware_refactor branch May 6, 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.