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

Jm mb 506 part2 zero down migration #3438

Merged
merged 6 commits into from Feb 10, 2020

Conversation

@jacquelineIO
Copy link
Contributor

jacquelineIO commented Jan 31, 2020

Description

2nd migration to remove the column service_item_id from payment_service_item table.
The is a follow-on to PR #3413

The code for the requested database change is in PR #3401

Reviewer Notes

n/a

Setup

Add any steps or code to run in this section to help others prepare to run your code:

make db_dev_reset db_dev_migrate

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).
  • 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 Jira acceptance criteria been met for this change?

References

Screenshots

n/a

DROP CONSTRAINT payment_service_items_service_item_id_fkey,
DROP COLUMN service_item_id,
ALTER COLUMN mto_service_item_id SET NOT NULL,
ADD CONSTRAINT payment_service_items_mto_service_item_id_fkey FOREIGN KEY (mto_service_item_id) REFERENCES mto_service_items (id);

This comment has been minimized.

Copy link
@reggieriser

reggieriser Jan 31, 2020

Contributor

In #3413, we added the new mto_service_item_id column and copied the service_item_id value into that new column for every payment_service_items record. There were no code changes with that PR. So wouldn't the code have still been writing to service_item_id in payment_service_items? And if that's the case, then wouldn't we be losing the service_item_id/mto_service_item_id for any payment_service_items added between PRs when we drop the service_item_id column above? Or am I missing something?

This comment has been minimized.

Copy link
@jacquelineIO

jacquelineIO Jan 31, 2020

Author Contributor

@reggieriser I think you are right. Should I do that copy again here?
This did cross my mind and I forgot :-/

This comment has been minimized.

Copy link
@jacquelineIO

jacquelineIO Jan 31, 2020

Author Contributor

However, none of this is actually live so it's not an issue. But I would like to get it right.

This comment has been minimized.

Copy link
@reggieriser

reggieriser Jan 31, 2020

Contributor

Agree that others may look to this as an example pattern to follow, though. I think the docs state to do it the way you did, too, so we should probably clarify that.

This comment has been minimized.

Copy link
@jacquelineIO

jacquelineIO Jan 31, 2020

Author Contributor

Updated this migration to do the copy again, but wondering if I should do what I'm doing or maybe even only update if mto_service_item_id is NULL like:

UPDATE payment_service_items SET mto_service_item_id = service_item_id
    WHERE mto_service_item_id IS NULL;

instead of what I picked which was:

UPDATE payment_service_items SET mto_service_item_id = service_item_id
    WHERE service_item_id IS NOT NULL;

This comment has been minimized.

Copy link
@reggieriser

reggieriser Feb 4, 2020

Contributor

Do you need the where clause at all? In theory, you don't know if the service_item_id has changed between the first PR and this one, so you could lose data if you only update a subset of records (although in practice, that's unlikely right now). Could you just update all the records again since you aren't using mto_service_id in the code until this PR?

Ignore the above in relation to this PR -- I was confused about the ordering of some related PRs. #3401 comes first, then this one.

This comment has been minimized.

Copy link
@reggieriser

reggieriser Feb 10, 2020

Contributor

I think the way you have the UPDATE currently is the right way to do this given the order of the PRs so far.

…ere deploy, update mto_serivce_id again before dropping service id column
README.md Outdated Show resolved Hide resolved
jacquelineIO and others added 2 commits Feb 6, 2020
fix to doc link

Co-Authored-By: Chris Gilmer <chris@truss.works>
Copy link
Contributor

reggieriser left a comment

LGTM! :shipit:

DROP CONSTRAINT payment_service_items_service_item_id_fkey,
DROP COLUMN service_item_id,
ALTER COLUMN mto_service_item_id SET NOT NULL,
ADD CONSTRAINT payment_service_items_mto_service_item_id_fkey FOREIGN KEY (mto_service_item_id) REFERENCES mto_service_items (id);

This comment has been minimized.

Copy link
@reggieriser

reggieriser Feb 10, 2020

Contributor

I think the way you have the UPDATE currently is the right way to do this given the order of the PRs so far.

@jacquelineIO jacquelineIO merged commit 914096b into master Feb 10, 2020
15 checks passed
15 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_storybook_app 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: check_generated_code 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
@jacquelineIO jacquelineIO deleted the jm-mb-506-part2-zero-down-migration branch Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.