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

Fetch access code #2352

Merged
merged 105 commits into from Jul 9, 2019

Conversation

@chrisrcoles
Copy link
Contributor

commented Jul 5, 2019

Description

Final PR that will activate access codes. See design doc for more detailed context.

Reviewer Notes

This reopens PR #2287.

Setup

Make sure .envrc.local has REQUIRES_ACCESS_CODE set to true.

1. make server_run
2. make client_run
3. go run cmd/generate_access_codes/main.go -ppm 20 -hhg 20
4. login as new Milmove user 

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 - Message to be sent 7/8 AM when deploy happens
  • There are no aXe warnings for UI.
  • This works in Supported Browsers and their phone views (Chrome, Firefox, IE, Edge).
  • 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

ralren and others added some commits May 8, 2019

Modify ValidatedPrivateRoute; create AccessCode as temporary redirect…
…; and have requires_access_code default to false.
WIP
with time zone NOT NULL DEFAULT NOW
(),
claimed_at timestamp
with time zone

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Contributor

So you're in a bit of a mess here if you've already run this on Experimental. Because modifying this file won't modify the experimental DB and will get us out of alignment. To add UNIQUE as an index you'll need to add a new migration file with a new name. Then we'll be certain that Experimental is the same as the other environments.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Contributor

Actually I'd prefer this file not be modified at all, even with the new formatting. I'm pretty sure it's already been deployed to staging and production.

This comment has been minimized.

Copy link
@kahlouie

kahlouie Jul 5, 2019

Contributor

I agree with @chrisgilmerproj 's reason on the second comment. I'd suggest if the claimed_at needs to have time zone, we should create a new migration to update it.

This comment has been minimized.

Copy link
@chrisrcoles

chrisrcoles Jul 5, 2019

Author Contributor

I agree with both of you - the only change here we are doing is adding the unique attribute to service_member_id so no other migration is needed if we decide to drop this change as @chrisgilmerproj suggests. I'll remove it and update the line formatting.

This comment has been minimized.

Copy link
@chrisrcoles

chrisrcoles Jul 5, 2019

Author Contributor

@kahlouie claimed_at already has the timezone - it's a line formatting difference that you're seeing

@@ -0,0 +1 @@
add_column("service_members", "requires_access_code", "bool", {"default": false})

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Contributor

Setting {"default": false} is going to lock the service member table while its added. I'd prefer that we added the column with no default (ie allowing null) and then come back and update each row individually so we don't lock. Once that's done we can alter the table to set NOT NULL on it.

This comment has been minimized.

Copy link
@chrisrcoles

chrisrcoles Jul 5, 2019

Author Contributor

Ok to clarify your point, I need to:

  1. remove require_access_code’s {"default": false} from the migration
  2. create another PR that just adds updates each row to set default to false and not null or is it okay to do this other migration in the same PR?
@chrisgilmerproj
Copy link
Contributor

left a comment

I've got some changes for migrations and feature flags I'd like to see on this. Also some minor cleanup to the circleci file.

@kahlouie
Copy link
Contributor

left a comment

@chrisrcoles This looks pretty good. One thing I talked with @jim about was maybe having a pr ready to deploy to reverse the feature flag in production considering the potential for limiting access to the point of the app being unusable. This would be ensuring we are making type 2 decisions (reversible ones). I have strong confidence in this work, but I think it would be good for us to use best practices when doing work that has the potential (however slight) to break important functionality of the app for users.

with time zone NOT NULL DEFAULT NOW
(),
claimed_at timestamp
with time zone

This comment has been minimized.

Copy link
@kahlouie

kahlouie Jul 5, 2019

Contributor

I agree with @chrisgilmerproj 's reason on the second comment. I'd suggest if the claimed_at needs to have time zone, we should create a new migration to update it.

@chrisrcoles

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

@kahlouie - thanks for the review and the suggestion. So I definitely have some feelings on what you brought up. This code has been designed, planned, tested on experimental, has been code reviewed numerous times, and tested extensively. Things will undoubtedly go wrong in engineering but when they do, we should respond swiftly and appropriately. While having the benefit of type 2 decision making, creating that PR implies that we do not have confidence in the code that we are releasing and are operating in an environment of fear - both of which are not healthy. If you have some reason why you are not confident in the release of that code, let's deal with that first. The feature flags themselves are very easy to turn on and off as it is simply a boolean value.

Chris Coles

@chrisrcoles chrisrcoles force-pushed the ren-#166649949-fetch-access-code branch from 194f840 to b2638d5 Jul 5, 2019

// Feature Flags
cli.InitFeatureFlag(flag)

// Don't sort flags

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Contributor

You'll want to revert the comment too :)

package cli

func (suite *cliTestSuite) TestCheckFeatureFlag() {
suite.Setup(InitDPSFlags, []string{})

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Contributor
Suggested change
suite.Setup(InitDPSFlags, []string{})
suite.Setup(InitFeatureFlagsFlag, []string{})
)

// InitFeatureFlag initializes FeatureFlags command line flags
func InitFeatureFlag(flag *pflag.FlagSet) {

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Contributor

Usually this is pluralized:

Suggested change
func InitFeatureFlag(flag *pflag.FlagSet) {
func InitFeatureFlags(flag *pflag.FlagSet) {
@@ -13,3 +13,4 @@ DB_SSL_MODE=verify-full
DB_SSL_ROOT_CERT=/bin/rds-combined-ca-bundle.pem
DEVLOCAL_CA=/config/tls/devlocal-ca.pem
DEVLOCAL_AUTH=false
REQUIRES_ACCESS_CODE=false

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Contributor
Suggested change
REQUIRES_ACCESS_CODE=false
FEATURE_FLAG_ACCESS_CODE=false
@@ -13,3 +13,4 @@ DB_SSL_MODE=verify-full
DB_SSL_ROOT_CERT=/bin/rds-combined-ca-bundle.pem
DEVLOCAL_CA=/config/tls/devlocal-ca.pem
DEVLOCAL_AUTH=false
REQUIRES_ACCESS_CODE=true

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Contributor
Suggested change
REQUIRES_ACCESS_CODE=true
FEATURE_FLAG_ACCESS_CODE=true
@@ -13,3 +13,4 @@ DB_SSL_MODE=verify-full
DB_SSL_ROOT_CERT=/bin/rds-combined-ca-bundle.pem
DEVLOCAL_CA=/config/tls/devlocal-ca.pem
DEVLOCAL_AUTH=false
REQUIRES_ACCESS_CODE=false

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Contributor
Suggested change
REQUIRES_ACCESS_CODE=false
FEATURE_FLAG_ACCESS_CODE=false
@@ -1,4 +1,5 @@
CREATE TABLE access_codes (
CREATE TABLE access_codes
(

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Contributor

I'd still revert this change

@@ -47,6 +47,8 @@ type HandlerContext interface {
SetSendProductionInvoice(sendProductionInvoice bool)
UseSecureCookie() bool
SetUseSecureCookie(useSecureCookie bool)
SetFeatureFlags(flags ...FeatureFlag)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Contributor

Why not just SetFeatureFlag? It seems like you run the risk of having some piece of code totally rewrite all the flags if all they want to do is set one flag. Alternatively, why not have GetFeatureFlags? Allowing someone to get all of them? This API feels like it ought to either do pluralized or singular actions but not one of each.

This comment has been minimized.

Copy link
@chrisrcoles

chrisrcoles Jul 8, 2019

Author Contributor

agreed! I've updated this!

@chrisgilmerproj
Copy link
Contributor

left a comment

The migration looks great! I did catch a few more things that ought to be fixed up before you deploy. Also a question about the API for feature flags.

Chris Coles
@kahlouie
Copy link
Contributor

left a comment

:shipit:

Chris Coles
// Feature Flags
cli.InitFeatureFlags(flag)

// Sort flags

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 9, 2019

Contributor

I think the original comment here was better.

Suggested change
// Sort flags
// Sort command line flags
@chrisgilmerproj
Copy link
Contributor

left a comment

Nice job. Please fix the comment in cmd/milmove/serve.go. I also don't see the UNIQUE constraint in a migration. I assume either you decided against it or it wasn't needed in this PR. But if you add it back then I'd like to review before you merge.

@chrisrcoles chrisrcoles force-pushed the ren-#166649949-fetch-access-code branch from ef988e8 to 645ecf5 Jul 9, 2019

@chrisrcoles chrisrcoles force-pushed the ren-#166649949-fetch-access-code branch from 645ecf5 to e91b73f Jul 9, 2019

@codecov

This comment has been minimized.

Copy link

commented Jul 9, 2019

Codecov Report

Merging #2352 into master will decrease coverage by 0.32%.
The diff coverage is 71.15%.

@@            Coverage Diff             @@
##           master    #2352      +/-   ##
==========================================
- Coverage   59.83%   59.51%   -0.32%     
==========================================
  Files         264      264              
  Lines       14792    14649     -143     
==========================================
- Hits         8850     8718     -132     
+ Misses       4914     4908       -6     
+ Partials     1028     1023       -5

@chrisrcoles chrisrcoles merged commit 76bdeb2 into master Jul 9, 2019

19 checks passed

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_tasks 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 71.15% of diff hit (target 59.83%)
Details
codecov/project/go Absolute coverage decreased by -0.32% but relative coverage increased by +11.48% compared to 5f4e61a
Details

@kahlouie kahlouie deleted the ren-#166649949-fetch-access-code branch Jul 19, 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.