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

Suppress bad moves in list: 164439301 #1959

Open
wants to merge 6 commits into
base: master
from

Conversation

3 participants
@lynzt
Copy link
Contributor

lynzt commented Apr 3, 2019

Description

Suppress bad/testing moves from showing in office and tsp queues list.

NOTE: The secure files for all 3 environments are different.
staging: the file is empty
experimental: the file should suppress a single move (move id: '6157910d-d8bd-4a79-98c5-a31089351386' -- "adsf, adsfsa")
production: the file should suppress known testing data (and is around 1035 lines long)

Setup

on master: make db_dev_e2e_populate

on this branch:
uncomment the update lines in 20190403150700_update-moves-show-flag.sql
db_dev_migrate

Ensure the moves listed in 20190403150700_update-moves-show-flag.sql don't show in the queue for the office and tsp.

Code Review Verification Steps

  • 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
  • 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

@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 3, 2019

Codecov Report

Merging #1959 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1959      +/-   ##
==========================================
- Coverage   60.56%   60.51%   -0.04%     
==========================================
  Files         193      193              
  Lines       12491    12500       +9     
==========================================
  Hits         7564     7564              
- Misses       4044     4053       +9     
  Partials      883      883

@lynzt lynzt requested review from chrisgilmerproj and tinyels Apr 3, 2019

@lynzt lynzt requested review from sarboc and reggieriser Apr 3, 2019

-- This will be run on development environments. It should mirror what you
-- intend to apply on production, but do not include any sensitive data.

-- * Leaving empty- modifying production moves show status not needed on development *

This comment has been minimized.

Copy link
@lynzt

lynzt Apr 3, 2019

Author Contributor

idk if we can test this locally since our e2e data is loaded after the migrations run so we don't have any move data in the db to test this on.

Now that I'm thinking about this, I could add a record to ensure updating the flag works locally, thoughts?

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Apr 3, 2019

Contributor

Good idea to test it locally.

@lynzt lynzt unassigned sarboc Apr 4, 2019

@lynzt lynzt force-pushed the lt-supress-bad-moves-164439301 branch from 86abff9 to a4ea437 Apr 4, 2019

@lynzt lynzt requested a review from mikena-truss Apr 4, 2019

@lynzt lynzt added the ttv label Apr 4, 2019

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

chrisgilmerproj commented Apr 4, 2019

Could you merge master so I could get #1967 in here to play with? Would make it a little easier to test. Thanks!

@lynzt lynzt force-pushed the lt-supress-bad-moves-164439301 branch from a4ea437 to 2b8172f Apr 8, 2019

@lynzt

This comment has been minimized.

Copy link
Contributor Author

lynzt commented Apr 8, 2019

@chrisgilmerproj - rebased master...

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

chrisgilmerproj commented Apr 8, 2019

@lynzt - I needed a few more tools so I brought this up to date.

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

I've tested the migrations and they work as expected in all environments.

I would love to see an e2e test added to this where you add a move to the DB that is intentionally suppressed and then check the queue to ensure it isn't there. Alternatively, you could make unit tests in golang that prove this feature works. Or both! But adding tests here would be good.

Thanks for being patient with me while I reviewed your migrations.

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.