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

update moves and delete duty stations w/ no zip3: 167301662 #2497

Merged
merged 2 commits into from Aug 15, 2019

Conversation

@lynzt
Copy link
Contributor

commented Aug 7, 2019

Description

Cleanup of this pr: #2397
During this pr, we discovered some of the moves on staging and prod had a foreign key constraint.

This migration updates the (origin/destination) duty station to another duty station. Then we delete duty stations that have no zip3 rows.

Reviewer Notes

Hard to verify locally since in our prior pr, we removed the duty stations w/o zip3 data. This is to clean up staging and prod.

Setup

make db_dev_reset
make db_dev_migrate

Code Review Verification Steps

  • Any new migrations/schema changes:
  • 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

@lynzt lynzt changed the title change moves with duty stations, delete duty stations w/ no zip3 update moves with duty stations and delete duty stations w/ no zip3: 167301662 Aug 7, 2019

@lynzt lynzt changed the title update moves with duty stations and delete duty stations w/ no zip3: 167301662 update moves and delete duty stations w/ no zip3: 167301662 Aug 7, 2019

@lynzt lynzt added the ttv label Aug 7, 2019

@chrisgilmerproj chrisgilmerproj requested review from mr337 and rdhariwal Aug 12, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

If this is supposed to only affect production systems should it be a secure migration?

@sarboc

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

@chrisgilmerproj My understanding was that secure migrations were meant to obscure private data, not an indication of where a migration should be run. Since there's no private data here, a regular migration should be fine.

@sarboc

sarboc approved these changes Aug 13, 2019

Copy link
Contributor

left a comment

@lynzt I'm approving this contingent on it being tested on experimental. If you did that already, can you check that box in the PR description. Thanks!

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

@chrisgilmerproj My understanding was that secure migrations were meant to obscure private data, not an indication of where a migration should be run. Since there's no private data here, a regular migration should be fine.

It serves two purposes: 1) Obscure sensitive data, 2) Protect each environment from the other.

So if this migration should be applied to each environment and local development then its fine as is. But if its not supposed to run in Experimental or if the UUIDs are different on Experimental than on Staging/Prod then we should use secure-migrations. I'm mostly looking for clarification here and making sure we're not inadvertently modifying data we don't intend to in each environment.

@lynzt

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@chrisgilmerproj - This update is for all db environments, not just staging and prod.

We are running this because there were 1 or more moves in those databases that used duty stations we were trying to delete. This pr updates the moves to use another duty station so we can delete duty stations without running into constraint errors (on staging and prod).

ping me if you have questions... might be quicker to talk it out... 😄

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

@chrisgilmerproj - This update is for all db environments, not just staging and prod.

That answers my question!

@chrisgilmerproj
Copy link
Contributor

left a comment

🚀

@lynzt lynzt merged commit 9e45139 into master Aug 15, 2019

16 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 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/go Coverage not affected when comparing 0628ee6...695eb46
Details
codecov/project/go 59.9% remains the same compared to 0628ee6
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.