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.

Sign up for GitHub

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

Jump to bottom

Support go embed #164

Closed
wants to merge 5 commits into from
Closed

Support go embed #164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d18189d
accept source.Driver when migrate
alovak May 8, 2021
9ef1384
add iofs as migrations source
alovak May 9, 2021
f07f3d9
switch to go 1.16
alovak May 10, 2021
1f7f7dc
support loading config from io.Reader
alovak May 10, 2021
2565f41
add some debug information to migrator logs
alovak May 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package config

import (
"fmt"
"io"
"os"
"strings"

Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Not returning the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@alovak alovak May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We have to re-do how we do configs as we now have to pass file (or its content) outside of the config package.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

if err := LoadEnvironmentFile(s.logger, APP_CONFIG, config); err != nil {
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Use our logger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It duplicates information in logs. I changed it as I saw the output and was duplicating the same errors...

}

deflt := viper.New()
deflt.SetConfigType("yaml")
if err := deflt.ReadConfig(f); err != nil {
return fmt.Errorf("unable to load the defaults: %w", err)
}

if err := deflt.Unmarshal(config); err != nil {
return fmt.Errorf("unable to unmarshal the defaults: %w", err)
}

return nil
}

func (s *Service) LoadFromReader(in io.Reader, config interface{}) error {
logger := s.logger
logger.Info().Logf("loading config file from reader")

deflt := viper.New()
deflt.SetConfigType("yaml")
if err := deflt.ReadConfig(in); err != nil {
return logger.LogErrorf("unable to load the defaults: %w", err).Err()
}

Expand Down
19 changes: 19 additions & 0 deletions config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package config_test

import (
"embed"
"os"
"testing"

Expand All @@ -17,6 +18,7 @@ type ConfigModel struct {
Default string
App string
Secret string
Custom string
Copy link
Member

Choose a reason for hiding this comment

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

Whats this?

Copy link
Contributor Author

@alovak alovak May 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Just to test that we read a custom field from a custom file.

Copy link
Member

Choose a reason for hiding this comment

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

The App is the custom file?

}

func Test_Load(t *testing.T) {
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

This PR only adds embed to our test file so far?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


func Test_LoadFile(t *testing.T) {
cfg := &GlobalConfigModel{}

service := config.NewService(log.NewDefaultLogger())

file, err := configs.Open("testdata/config.yml")
require.NoError(t, err)

err = service.LoadFromReader(file, cfg)
require.NoError(t, err)

require.Equal(t, "custom", cfg.Config.Custom)
}
2 changes: 2 additions & 0 deletions config/testdata/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Config:
Custom: "custom"
2 changes: 1 addition & 1 deletion database/database.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func NewAndMigrate(ctx context.Context, logger log.Logger, config DatabaseConfig
}

// run migrations first
if err := RunMigrations(logger, config); err != nil {
if err := RunMigrations(logger, config, nil); err != nil {
return nil, err
}

Expand Down
29 changes: 27 additions & 2 deletions database/migrator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to keep base package compatible with old versions of the service, I can create RunMigrationsWithSource to support pkger migrations as well.

db, err := New(context.Background(), logger, config)
if err != nil {
return err
Expand All @@ -31,9 +31,13 @@ func RunMigrations(logger log.Logger, config DatabaseConfig) error {
return err
}

if migrationsSource != nil {
source = migrationsSource
}

migrationMutex.Lock()
m, err := migrate.NewWithInstance(
"filtering-pkger",
"migrations",
source,
config.DatabaseName,
driver,
Expand All @@ -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()
Copy link
Member

Choose a reason for hiding this comment

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

Why was this added in? m.Up() brings it upto the latest version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

switch {
case err == migrate.ErrNilVersion:
logger.Info().Log("Clean DB, no migrations")
case err != nil:
return logger.Fatal().LogErrorf("Error getting current migration version: %w", err).Err()
default:
logger.Info().Logf("DB is at version: %d", ver)
}

err = m.Up()

migrationMutex.Unlock()

switch err {
Expand All @@ -53,6 +68,16 @@ func RunMigrations(logger log.Logger, config DatabaseConfig) error {
return logger.Fatal().LogErrorf("Error running migrations: %w", err).Err()
}

ver, _, err = m.Version()
switch {
case err == migrate.ErrNilVersion:
logger.Info().Log("Clean DB, no migrations")
case err != nil:
return logger.Fatal().LogErrorf("Error getting current migration version: %w", err).Err()
default:
logger.Info().Logf("DB migrated to version: %d", ver)
}

logger.Info().Log("Migrations complete")

return nil
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/moov-io/base

go 1.13
go 1.16

require (
github.com/go-kit/kit v0.10.0
Expand Down