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

MB-2799 Import fake pricing data to staging/experimental #4255

Merged
merged 16 commits into from
Jun 22, 2020

Conversation

noelledusahel
Copy link
Contributor

@noelledusahel noelledusahel commented Jun 15, 2020

Description

Follows steps outlined in https://dp3.atlassian.net/browse/MB-2262 to import and upload fake pricing data via secure migration

  • WIll not be deployed to production

  • Deployed to Staging & Experimental only

Steps for deploying to experimental:

Reviewer Notes

Look for 20200615192021_import_pricing_data_june_2020.up.sql in the following folders

  • transcom-ppp-app-experimental-us-west-2/secure-migrations
  • transcom-ppp-app-staging-us-west-2/secure-migrations
  • transcom-ppp-app-prod-us-west-2/secure-migrations (this file should only contain a comment, no sql)

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 #g-database
    • Secure migrations have been tested following the instructions in our docs
  • 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

If this PR makes visible UI changes, an image of the finished UI can help reviewers and casual
observers understand the context of the changes. A before image is optional and
can be included at the submitter's discretion.

Consider using an animated image to show an entire workflow instead of using multiple images. You may want to use GIPHY CAPTURE for this! 📸

Please frame screenshots to show enough useful context but also highlight the affected regions.

@noelledusahel noelledusahel requested a review from a team June 15, 2020 21:47
@robot-mymove
Copy link

robot-mymove commented Jun 15, 2020

Messages
📖 🔗 MB-2799

Generated by 🚫 dangerJS against 9cb6b11

@noelledusahel noelledusahel marked this pull request as draft June 15, 2020 22:50
@noelledusahel noelledusahel marked this pull request as ready for review June 16, 2020 01:13
Copy link
Contributor

@blvrd blvrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2020-06-16 at 12 36 30
😮

Looks good to me!

Copy link
Contributor

@reggieriser reggieriser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the contract code is set to Pricing. Is that what we want for the test data? I'm thinking we may need to make it more obvious this is test data rather than real pricing. Also, my PR #4250 assumed it was named TRUSS (which I just made up), so I'll need to change that accordingly to whatever the name is here.

Note that if we do decide to change the code, we'll need to restore the experimental DB to make the migration run again there (infra can help us with that -- we've done it several times before).

@noelledusahel
Copy link
Contributor Author

It looks like the contract code is set to Pricing. Is that what we want for the test data? I'm thinking we may need to make it more obvious this is test data rather than real pricing. Also, my PR #4250 assumed it was named TRUSS (which I just made up), so I'll need to change that accordingly to whatever the name is here.

Note that if we do decide to change the code, we'll need to restore the experimental DB to make the migration run again there (infra can help us with that -- we've done it several times before).

OK i definitely missed that the code 'Pricing' stuck, i meant for it to be 'june_2020' @jacquelineIO do you have any guidance on what Contract Code we want attached to this fake data?

@reggieriser
Copy link
Contributor

I went ahead and had infra roll back the experimental database so we can run this migration against experimental again when we're ready (let's make it the last thing we do after reviewer approval but before merging).

See #4279 for a fix that will need to be merged into this PR after #4279 is approved.

Also, I think we should consider changing the name of the migration since import_pricing_data_june_2020 could imply this is real data. I'm thinking more like import_fake_pricing_data. We still need to get confirmation of the contract code we want to use.

With the fix from #4279, I checked all the re_* tables imported from this PR against a psql-loaded migration and they seemed to match. 🎉

@noelledusahel
Copy link
Contributor Author

I went ahead and had infra roll back the experimental database so we can run this migration against experimental again when we're ready (let's make it the last thing we do after reviewer approval but before merging).

See #4279 for a fix that will need to be merged into this PR after #4279 is approved.

Also, I think we should consider changing the name of the migration since import_pricing_data_june_2020 could imply this is real data. I'm thinking more like import_fake_pricing_data. We still need to get confirmation of the contract code we want to use.

With the fix from #4279, I checked all the re_* tables imported from this PR against a psql-loaded migration and they seemed to match. 🎉

to change the name of the migraiton to import_fake_pricing_data_ i would need to write code in the script that differentiates between an upload of fake data vs real data. i guess it would look like an extra step in the script that asks the user whether this is real or fake data. would it be better to just give the fake data a contract code that indicates that its fake? like (Fake Truss Data)

@reggieriser
Copy link
Contributor

to change the name of the migraiton to import_fake_pricing_data_ i would need to write code in the script that differentiates between an upload of fake data vs real data. i guess it would look like an extra step in the script that asks the user whether this is real or fake data. would it be better to just give the fake data a contract code that indicates that its fake? like (Fake Truss Data)

I don't think the script has to change. You should be able to change the migration filename after the pricing-import script creates it -- just remember to also change the migrations_manifest.txt file accordingly. I'm also fine with import_pricing_data_fake (if you want to keep the same prefix) or whatever you'd like that makes it clear that it's not real data. That reminds me -- we will also need to clean up S3 to remove the old migration in each environment and add the new one when it's ready.

@noelledusahel
Copy link
Contributor Author

to change the name of the migraiton to import_fake_pricing_data_ i would need to write code in the script that differentiates between an upload of fake data vs real data. i guess it would look like an extra step in the script that asks the user whether this is real or fake data. would it be better to just give the fake data a contract code that indicates that its fake? like (Fake Truss Data)

I don't think the script has to change. You should be able to change the migration filename after the pricing-import script creates it -- just remember to also change the migrations_manifest.txt file accordingly. I'm also fine with import_pricing_data_fake (if you want to keep the same prefix) or whatever you'd like that makes it clear that it's not real data. That reminds me -- we will also need to clean up S3 to remove the old migration in each environment and add the new one when it's ready.

ok so you're talking about manually changing it right?

@reggieriser
Copy link
Contributor

reggieriser commented Jun 18, 2020

ok so you're talking about manually changing it right?

Right. But if you wanted to change the pricing-import script, I'd probably just give the user the option/flexibility to enter a migration name rather than asking them if it's fake data or not. But I think a manual change is fine for this story.

Copy link
Contributor

@reggieriser reggieriser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I tried a make run_staging_migrations and compared the re_* tables in my deployed_migrations database against my local DB that had an imported version of the fake data via the pricing parser. Everything aligned (and the problem with the lack of leading zeros was gone). I looked at S3 for each environment and saw the same migration in experimental/staging and a no-op in prod (as expected). :shipit:

In anticipation of this getting merged, I'll go ahead and update my PR #4250 to default to using the TRUSS_TEST contract code.

@noelledusahel noelledusahel merged commit 7bf15b0 into master Jun 22, 2020
@noelledusahel noelledusahel deleted the nb-MB-2799-import-fake-data branch June 22, 2020 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants