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

Use migrations manifest #2308

Merged
merged 3 commits into from Jul 10, 2019

Conversation

@pjdufour-truss
Copy link
Contributor

commented Jun 20, 2019

Description

Rather than running whatever files are in the migrations folders, milmove migrate will now error on any files that were not listed in the migrations manifest.

Added a pre-commit hook, that updates the manifest. To avoid issues with merge commits, the new pre-commit hook doesn't run in CI. However, if a commit is missing from the manifest the server tests will fail.

We'll have the added benefit of an easy to view changelog of migrations.

Reviewer Notes

Local Migrations

make db_dev_destroy && make db_dev_start && DB_TIMEOUT=30 make db_dev_create && make db_dev_migrate

Setup

None

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 Pivotal acceptance criteria been met for this change?

References

None

Screenshots

None

@pjdufour-truss pjdufour-truss force-pushed the migration_manifest branch from 8c0d759 to a2b23a7 Jun 20, 2019

@pjdufour-truss pjdufour-truss changed the title use migrations manifest Use migrations manifest Jun 20, 2019

@pjdufour-truss pjdufour-truss force-pushed the migration_manifest branch from 19aa03f to e4f676c Jun 21, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

I'd like to see unit tests for the new functions in pkg/migrate/. Also, let's deploy to experimental before we merge.

@pjdufour-truss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Hard to see how to add unit tests..., since fizz automatically executes migrations as they are parsed. The new functions were copied out of the pop repo and edited only slightly.

@pjdufour-truss

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Now requires gobuffalo/pop#407

@pjdufour-truss pjdufour-truss force-pushed the migration_manifest branch from e4f676c to 700ac51 Jul 1, 2019

@chrisgilmerproj chrisgilmerproj self-requested a review Jul 1, 2019

go.mod Outdated
gopkg.in/alexcesaro/quotedprintable.v3 v3.0.0-20150716171945-2caba252f4dc // indirect
gopkg.in/alexcesaro/statsd.v2 v2.0.0 // indirect
gopkg.in/gomail.v2 v2.0.0-20160411212932-81ebce5c23df // indirect
gopkg.in/urfave/cli.v1 v1.20.0 // indirect
)

replace github.com/gobuffalo/pop => github.com/pjdufour-truss/pop v4.11.2-0.20190611171623-8b1bc4b6292d+incompatible

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal Jul 1, 2019

Contributor

Is this a place holder until this PR gobuffalo/pop#407 merges?

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss Jul 2, 2019

Author Contributor

yes

@chrisgilmerproj
Copy link
Contributor

left a comment

🚀 - Let's get someone on ARBOC to take a look as well, but this PR works for me.

@pjdufour-truss pjdufour-truss force-pushed the migration_manifest branch from 700ac51 to 0e9fc96 Jul 2, 2019

@pjdufour-truss pjdufour-truss force-pushed the migration_manifest branch from 0e9fc96 to e8b9665 Jul 5, 2019

@pjdufour-truss pjdufour-truss requested a review from lynzt Jul 8, 2019

@pjdufour-truss pjdufour-truss force-pushed the migration_manifest branch from 213ef5f to 6ed6469 Jul 9, 2019

@codecov

This comment has been minimized.

Copy link

commented Jul 9, 2019

Codecov Report

Merging #2308 into master will decrease coverage by 0.08%.
The diff coverage is 3.23%.

@@            Coverage Diff             @@
##           master    #2308      +/-   ##
==========================================
- Coverage   59.85%   59.77%   -0.08%     
==========================================
  Files         266      265       -1     
  Lines       14768    14807      +39     
==========================================
+ Hits         8838     8850      +12     
- Misses       4902     4929      +27     
  Partials     1028     1028
@jim
Copy link
Contributor

left a comment

Nice work on this.

For anyone who is interested, here is what you see if you create a migration and attempt to run it without updating the manifest:

jimb@wfh ~/c/mymove (migration_manifest) [2]> make db_dev_migrate
Migrating the dev_db database...
# We need to move to the scripts/ directory so that the cwd contains `apply-secure-migration.sh`
cd scripts && \
		../bin/milmove migrate -p ../migrations -m ../migrations_manifest.txt
2019-07-09T18:24:48.320Z	INFO	milmove/migrate.go:138	migrator starting up	{"git_branch": "migration_manifest", "git_commit": "213ef5f2a968c9f00255444a4cb99f14da11b9ab"}
2019-07-09T18:24:48.320Z	INFO	milmove/migrate.go:50	checking migration config	{"git_branch": "migration_manifest", "git_commit": "213ef5f2a968c9f00255444a4cb99f14da11b9ab"}
2019-07-09T18:24:48.320Z	INFO	cli/dbconn.go:225	Connecting to the database	{"git_branch": "migration_manifest", "git_commit": "213ef5f2a968c9f00255444a4cb99f14da11b9ab", "url": "postgres://postgres:*****@localhost:5432/dev_db?sslmode=disable", "db-ssl-root-cert": ""}
2019-07-09T18:24:48.327Z	INFO	milmove/migrate.go:163	using migration path "../migrations"	{"git_branch": "migration_manifest", "git_commit": "213ef5f2a968c9f00255444a4cb99f14da11b9ab"}
2019-07-09T18:24:48.327Z	INFO	milmove/migrate.go:166	using migration manifest at "../migrations_manifest.txt"	{"git_branch": "migration_manifest", "git_commit": "213ef5f2a968c9f00255444a4cb99f14da11b9ab"}
panic: migration at path "../migrations/20190709182439_blahblahblah.down.fizz" missing from manifest

goroutine 1 [running]:
main.main()
	/Users/jimb/code/mymove/cmd/milmove/main.go:69 +0x583
make: *** [db_dev_migrate_standalone] Error 2

To fix that, you can run scripts/update-migrations-manifest.

I'd like to see us add a scripts/gen-migration script that creates the migration and then runs the update script automatically so that this isn't something that developer will have to do manually every time they create a new migration. We have similar scripts for generating migrations for other environments already, so this would follow that pattern.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

@jim - would just modifying this script help? https://github.com/transcom/mymove/blob/master/scripts/gen-model . I'm thinking scripts/gen-migration might be as simple as touch migrations/$(date '+%Y%m%d')_new_migration_name.up.fizz && scripts/update-migrations-manifest

@jim

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

@chrisgilmerproj I don't think we generate models that much anymore, but we do create lots of other migrations. We could probably replace gen-model with gem-migration if we wanted.

func InitMigrationFileFlags(flag *pflag.FlagSet) {
flag.String(MigrationVersionFlag, time.Now().Format(VersionTimeFormat), "migration version: integer representation of datetime, default is current time using Go format "+VersionTimeFormat)
flag.StringP(MigrationNameFlag, "n", "", "migration name: alphanumeric, no spaces, underscores and dashes allowed")
flag.StringP(MigrationTypeFlag, "t", "", "migration type: fizz or sql.")

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 9, 2019

Contributor

I think I'd prefer that this had a default and that it was fizz which is what most people use.

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss Jul 9, 2019

Author Contributor

Done

// MigrationTypeFlag is the migration manifest flag
MigrationTypeFlag string = "type"

VersionTimeFormat string = "20060102150405"

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 9, 2019

Contributor

Needs a comment here :)

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss Jul 9, 2019

Author Contributor

Done

Long: "Generate migrations and other objects",
RunE: nil,
}
initMigrateFlags(genCommand.Flags())

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 9, 2019

Contributor

Do we want milmove gen -h to actually have any options?

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss Jul 9, 2019

Author Contributor

Fixed

@chrisgilmerproj
Copy link
Contributor

left a comment

I tested out the new milmove gen migration command and its pretty slick. Thanks for adding that.

@jim

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

@pjdufour-truss I was expecting a shell script, so this is great!

@jim

jim approved these changes Jul 10, 2019

@pjdufour-truss pjdufour-truss force-pushed the migration_manifest branch from 10ba19e to 8c153ee Jul 10, 2019

@pjdufour-truss pjdufour-truss merged commit 964f35f into master Jul 10, 2019

17 of 19 checks passed

codecov/patch 3.23% of diff hit (target 59.85%)
Details
codecov/project/go 59.61% (-0.08%) compared to 035838c
Details
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_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp 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

@pjdufour-truss pjdufour-truss deleted the migration_manifest branch Jul 10, 2019

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.