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

Change make target db_dev_migrate to allow users to change the DB_NAME. #1538

Merged
merged 4 commits into from Jan 2, 2019

Conversation

3 participants
@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj commented Jan 2, 2019

Description

This PR does two things:

  • Change make target db_dev_migrate to allow users to change the DB_NAME.
  • Fix make target server_test to ensure we don't create, destroy, and create the DB and cause a race condition.

For the first:

The script bin/run-prod-migrations relies on the ability to override DB_NAME env var when calling make db_dev_migrate. This used to work and recent changes to the Makefile disabled it. The assumption was that calling db_dev_* targets would only ever modify the dev_db database. There are two ways to address, we could either make new db_prod_migrations targets to be more strict or we could revert to the old behavior. I've gone with the latter since it's easier and creates minimal changes.

For the second:

db_test_reset calls db_test_run after db_destroy. By calling run and then reset we essentially create, destroy, and create a DB and cause a race condition. By removing the unneeded call to db_test_run we get fixed up.

For more information, we recently made db_test_reset call db_destroy because of the way in which we have three DB's on a single container that all need to be accessed via localhost while simultaneously needing to be able to reset those DBs without causing the web app or e2e tests to throw a runtime error when the DB disappears.

Setup

Run Prod Migrations

Try:

./bin/run-prod-migrations

Test that the migrations were actually applied to the prod_migrations DB and not dev_db by introspecting the prod_migrations DB with your tool of choice.

Then try make db_dev_migrate and you'll see that it uses env var defaults from .envrc and correctly migrates dev_db.

Server Test

Try make server_test and it ought to work.

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@chrisgilmerproj chrisgilmerproj self-assigned this Jan 2, 2019

@chrisgilmerproj chrisgilmerproj requested review from akostibas and reggieriser Jan 2, 2019

@reggieriser
Copy link
Contributor

reggieriser left a comment

Followed your instructions successfully. LGTM!

Makefile Outdated
@@ -178,7 +178,7 @@ else
go test -p 1 -count 1 -short $$(go list ./... | grep \\/cmd\\/webserver) 2> /dev/null
endif

server_test: server_deps server_generate db_test_run db_test_reset db_test_migrate
server_test: server_deps server_generate db_test_reset db_test_migrate

This comment has been minimized.

@akostibas

akostibas Jan 2, 2019

Contributor

I encountered a problem with this and tried fixing in another PR. Server tests were failing because test database didn't exist. Looking more closely at the Makefile, db_test_reset includes a db_test_run, so I'm not sure why I was having that problem. But when I swapped the order, the problem went away. I assume there was some race condition?

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 2, 2019

Contributor

Yeah, there was a race condition. But we didn't need to so much swap the order as ensure that the db_test_run wasn't called immediately before destroying it (which db_test_reset does).

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 2, 2019

Contributor

At some point I had updated the description to this PR and it appears Github's outage lost it (or maybe I refreshed the page?). Anyway, I've tried to update it now.

@akostibas
Copy link
Contributor

akostibas left a comment

It feels like we have a gross global variable issue with DB_NAME, which I helped to create.

That said, this works for now. +1

@chrisgilmerproj chrisgilmerproj merged commit 99e97d7 into master Jan 2, 2019

12 checks passed

ci/circleci: acceptance_tests 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_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

@chrisgilmerproj chrisgilmerproj deleted the cg_162898668_fix_db_dev_migrate_for_run_prod_migrations branch Jan 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment