Support go embed #164
Support go embed #164
Conversation
Codecov Report
@@ Coverage Diff @@
## master #164 +/- ##
==========================================
- Coverage 79.76% 78.69% -1.07%
==========================================
Files 25 25
Lines 771 798 +27
==========================================
+ Hits 615 628 +13
- Misses 93 104 +11
- Partials 63 66 +3
Continue to review full report at Codecov.
|
| @@ -17,7 +17,7 @@ import ( | |||
|
|
|||
| var migrationMutex sync.Mutex | |||
|
|
|||
| func RunMigrations(logger log.Logger, config DatabaseConfig) error { | |||
| func RunMigrations(logger log.Logger, config DatabaseConfig, migrationsSource source.Driver) error { | |||
There was a problem hiding this comment.
If we want to keep base package compatible with old versions of the service, I can create RunMigrationsWithSource to support pkger migrations as well.
| @@ -45,12 +47,29 @@ func (s *Service) LoadFile(file string, config interface{}) error { | |||
|
|
|||
| f, err := pkger.Open(file) | |||
| if err != nil { | |||
| return logger.LogErrorf("pkger unable to load %s: %w", file, err).Err() | |||
| return fmt.Errorf("pkger unable to load %s: %w", file, err) | |||
There was a problem hiding this comment.
It duplicates information in logs. I changed it as I saw the output and was duplicating the same errors...
| @@ -25,7 +27,7 @@ func NewService(logger log.Logger) Service { | |||
|
|
|||
| func (s *Service) Load(config interface{}) error { | |||
| if err := s.LoadFile(pkger.Include("/configs/config.default.yml"), config); err != nil { | |||
| return err | |||
| s.logger.LogErrorf("loading default config file using pkger: %w", err) | |||
There was a problem hiding this comment.
Yeap. My idea here, if we want to keep pkger ... for backward compatibility?, just to ignore the error. And log it instead. I would prefer to get rid of pkger then we don't need it.
There was a problem hiding this comment.
If its going embedded completely we don't need it??? because the next compile should bring in the default. If it doesn't work that way we can't use the embed tooling yet then and have to re-do how we do configs.
There was a problem hiding this comment.
We have to re-do how we do configs as we now have to pass file (or its content) outside of the config package.
There was a problem hiding this comment.
It sounds like moovfinancial needs a config package to address the needs of those services, so moov-io/base might not be the best location for these changes.
| @@ -17,6 +18,7 @@ type ConfigModel struct { | |||
| Default string | |||
| App string | |||
| Secret string | |||
| Custom string | |||
There was a problem hiding this comment.
Just to test that we read a custom field from a custom file.
| @@ -42,7 +46,18 @@ func RunMigrations(logger log.Logger, config DatabaseConfig) error { | |||
| return logger.Fatal().LogErrorf("Error running migration: %w", err).Err() | |||
| } | |||
|
|
|||
| ver, _, err := m.Version() | |||
There was a problem hiding this comment.
Why was this added in? m.Up() brings it upto the latest version
There was a problem hiding this comment.
Based on current output I had no idea what was the version before migration and what's the version after. It was not clear for me if migrations run at all. That's why I decided to add some information to migrations.
| @@ -37,3 +39,20 @@ func Test_Load(t *testing.T) { | |||
| require.Equal(t, "app", cfg.Config.App) | |||
| require.Equal(t, "keep secret!", cfg.Config.Secret) | |||
| } | |||
|
|
|||
| //go:embed testdata/*.yml | |||
| var configs embed.FS | |||
There was a problem hiding this comment.
This PR only adds embed to our test file so far?
There was a problem hiding this comment.
Yeap. embed works for the package directory or subdirectories. Here I'm using this for tests.
templater has separate packages configs and migrations which embed files and pass them inside the config or migration.
|
LGTM overall, but we need to release this as v0.19.0 with a note in the changelog about the breaking change. https://github.com/moov-io/base/blob/master/CHANGELOG.md |
|
Replaced by #302 |
The goal of this PR is to switch Moov's projects from
pkgertogo:embedfor default config and migrations.Why?
pkgeris an external dependency that currently adds external steps in dev setup, compilation, etc. it creates unnecessary frictionpkgerraises some unnecessary questions like: should we storepkged.goin repository or no? (the answer is no, but in some repositories we still have it)Some context:
go:embedandgo mod tidy, it was fixed, butgolang-migrateteam decided to wait for a little and git time to users to switch to fixed go version) here are more details: the feat: embed, io/fs support golang-migrate/migrate#471io.fswhich we are using in the PRConcerns / Questions
pkgerfor previous projects? Or I can remove it completely?