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

refactor(storage/sql): embed migrations into flipt binary #1096

Merged
merged 5 commits into from
Oct 26, 2022

Conversation

GeorgeMac
Copy link
Contributor

@GeorgeMac GeorgeMac commented Oct 26, 2022

The core of this refactor is to move migrations into an embed.FS variable.
This will embed all migrations into the Flipt binary; making it more portable.

The PR:

  1. Creates the new package config/migrations containing a single exported variable FS with go:embed directive.
  2. Updates all the locations where we source migrations via file, to use the new source driver iofs.
  3. Deprecates both db.migrations.path and db.migrations_path configuration options.
  4. Removes all references internally to these deprecated paths.
  5. Removes references to config/migrations from packaging, archiving and docker image building.
  6. Does a little shuffling around of storage/sql to introduce a new re-usable storage/sql/testing package.

@GeorgeMac GeorgeMac changed the title reactor(storage/sql): embed migrations into flipt binary refactor(storage/sql): embed migrations into flipt binary Oct 26, 2022
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

Looking good, mostly just minor pedantic comments. Gonna pull down locally real quick to get a better look before 👍🏻 .

Also would you mind adding a section to the depcrecations file ? Also perhaps adding a section to the Changelog like:

## Unreleased

### Deprecated
- blah

Just so we don't forget before releasing?

In the future we could prob move to using release-please to update the changelog for us based on conventional commits, but we may need to do some configuration there first to make it match the current syntax of our Changelog file.

internal/config/deprecate.go Outdated Show resolved Hide resolved
internal/storage/sql/testing/testing.go Show resolved Hide resolved
internal/storage/sql/testing/testing.go Outdated Show resolved Hide resolved
@markphelps
Copy link
Collaborator

This is great btw, will prevent issues in the future with migrations misconfigurations/incorrect paths! TY!!

@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2022

Codecov Report

Merging #1096 (a7b276b) into main (56e3241) will decrease coverage by 0.28%.
The diff coverage is 84.52%.

@@            Coverage Diff             @@
##             main    #1096      +/-   ##
==========================================
- Coverage   80.80%   80.52%   -0.29%     
==========================================
  Files          26       26              
  Lines        1870     1884      +14     
==========================================
+ Hits         1511     1517       +6     
- Misses        279      287       +8     
  Partials       80       80              
Impacted Files Coverage Δ
internal/storage/sql/migrator.go 26.38% <0.00%> (-0.76%) ⬇️
internal/storage/sql/db.go 90.69% <68.42%> (-4.09%) ⬇️
internal/config/database.go 100.00% <100.00%> (ø)
internal/storage/sql/metrics.go 60.90% <100.00%> (+0.72%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

👨🏻‍🍳 💋

@GeorgeMac GeorgeMac merged commit 688c2ba into main Oct 26, 2022
@GeorgeMac GeorgeMac deleted the gm/migrations-embed branch October 26, 2022 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants