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

Upgrade fizz files to the latest spec #2506

Merged
merged 2 commits into from Aug 13, 2019

Conversation

@reggieriser
Copy link
Contributor

commented Aug 9, 2019

Description

Starting with Pop 4.11.x (we're currently on 4.11.2), Pop is adding a t.Timestamps() macro when generating a migration for a new model. This is in anticipation of fizz soon dropping the implicit adding of timestamp columns which we've been depending on to this point. That means applying existing migrations to a new database (like we do when we reset dev or test databases) would have no timestamp columns for fizz migrations when this fizz change lands. More details can be found in the Pop 4.11.0 release notes.

Pop provides a soda fix command to add this macro to existing fizz migrations. I used that to generate the needed changes. Also note that this fixed some inconsistencies we had with internal spacing/indenting and the occasional use of semicolons at the end of fizz commands (although they didn't seem to have affected our resulting schema).

The soda fix command also removed some blank lines when I ran it, but I put most of those back in because we were often using those blank lines to logically separate parts of the fizz file.

Reviewer Notes

I did make a change to the Pop config file (config/database.yml) because I was getting this warning when running soda fix:

warn - unable to load connection test: no database or URL specified

I'm wondering if TEST_DB_NAME should have been DB_NAME_TEST? I couldn't find any reference to TEST_DB_NAME in the project, but we set DB_NAME_TEST in .envrc. Is this a safe change? Is there somewhere else we're setting TEST_DB_NAME?

Setup

To test that this doesn't change the schema at all, I suggest first doing a make db_dev_reset db_dev_migrate on master. That will generate a schema.sql file. Rename it to schema.master.sql.

Then, checkout this PR's branch. Do a make db_dev_reset db_dev_migrate again. You should get another schema.sql file generated. Diff this one and the schema.master.sql one. There should be no diffs (unless a new migration that changes the schema has landed in master -- if that's the case, let me know and I'll update this PR to account for it).

If you want to see how soda fix is removing the blank lines I mentioned above (or that we've "fixed" everything), you can simply run soda fix on this branch and then diff the fizz migrations it modified. The only differences in those modified migrations should be the blank lines.

Code Review Verification Steps

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

References

@@ -13,7 +13,7 @@ development:
# This block is used by integration tests on CircleCI.
test:
dialect: "postgres"
database: {{ env "TEST_DB_NAME" }}
database: {{ env "DB_NAME_TEST" }}

This comment has been minimized.

Copy link
@reggieriser

reggieriser Aug 9, 2019

Author Contributor

This fixes the soda warning mentioned in the PR since we set DB_NAME_TEST in .envrc. But do we use the TEST_DB_NAME anywhere else? I couldn't find any other references in the code, but maybe there's another place the environment variable is set. @jim, it looks like you changed this to use TEST_DB_NAME a few weeks ago -- what's your take?

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Aug 12, 2019

Contributor

I don't know that we use that anywhere else. I think this is a golang template? The way to test this is to use one of our scripts that uses soda. Infra is trying to deprecate the use of this file all together and so the milmove app doesn't use it. So just running the server won't exercise this.

This comment has been minimized.

Copy link
@reggieriser

reggieriser Aug 12, 2019

Author Contributor

Based on what you're saying, it sounds like this is a safe change since I originally got the warning running soda fix (and making this change cleared it up).

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Aug 12, 2019

Contributor

yeah, sounds good!

@reggieriser reggieriser added the 🦇team label Aug 9, 2019

@lynzt
Copy link
Contributor

left a comment

I'm deferring approval to let infra weigh in w/ about env variable but LGTM...

@chrisgilmerproj / @rdhariwal / @mr337 - does database: {{ env "DB_NAME_TEST" }} look good to you?

@@ -1,94 +1,93 @@
add_index("backup_contacts", "email", {});
add_index("backup_contacts", "email", {})

This comment has been minimized.

Copy link
@lynzt

lynzt Aug 12, 2019

Contributor

so fizz doesn't need (shouldn't have) ;... good to know

This comment has been minimized.

Copy link
@reggieriser

reggieriser Aug 12, 2019

Author Contributor

Apparently. It's possible the confusion came from the fact that you typically do put a ; after a SQL statement in Postgres, but that semicolon would be inside the string passed to the fizz sql() function and not terminating a fizz statement.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Aug 12, 2019

Contributor

I didn't know this! Can you do an extra check to make sure we aren't including any semicolons where they aren't needed? Not to long ago Patrick looked for places where we were missing semicolons and found this one:

https://github.com/transcom/mymove/pull/2349/files#diff-a8947f3079ef42e99df348d1796cf6d8R3-R4

Might be worth fixing up any of these in this same PR.

This comment has been minimized.

Copy link
@reggieriser

reggieriser Aug 13, 2019

Author Contributor

That link took me to a page with a ton of diffs (anchor must not be working), so I'm not exactly sure of the context of the missing semicolons you're referring to. For this PR, I've removed (at soda fix's insistence) all the semicolons that were terminating fizz statements. We do have some cases where we haven't used a semicolon to terminate a sql string inside of a fizz sql() call, but I'm not sure that's technically incorrect unless you were sending multiple sql statements. Even so, I don't think I want to do too many manual adjustments like that in this PR -- if we think fixing those is something worth addressing, I suggest we do it in a separate story/PR.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Aug 13, 2019

Contributor

Sorry, an update to that branch messed it all up. Here's the fix pulled into its own PR: #2519

@chrisgilmerproj
Copy link
Contributor

left a comment

Did you also check the local migrations? do any secure migrations have fizz files?

@reggieriser

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Did you also check the local migrations? do any secure migrations have fizz files?

Yes. I looked at local migrations as well as the secure migrations in all three deployed environments and didn't see any fizz files. Not too surprising since the generate-secure-migration script outputs *.sql files and we're typically adding or manipulating data in them.

@codecov

This comment has been minimized.

Copy link

commented Aug 12, 2019

Codecov Report

Merging #2506 into master will decrease coverage by 0.1%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #2506     +/-   ##
========================================
- Coverage    59.7%   59.7%   -<.1%     
========================================
  Files         283     284      +1     
  Lines       15878   15893     +15     
========================================
  Hits         9476    9476             
- Misses       5285    5300     +15     
  Partials     1117    1117
Impacted Files Coverage Δ
pkg/handlers/converters_strfmt.go 0% <0%> (ø)
@chrisgilmerproj
Copy link
Contributor

left a comment

🚀 ! - Looks good. It's up to you what to do with the semicolon stuff.

@reggieriser reggieriser merged commit 856ae84 into master Aug 13, 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 78b74fc...b4d9436
Details
codecov/project/go 59.5% (-<.1%) compared to 78b74fc
Details

@reggieriser reggieriser deleted the rr-upgrade-fizz-files branch Aug 13, 2019

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.