From f2e74da5aa652ab2b9787b656ca7e8b2f67ee5a9 Mon Sep 17 00:00:00 2001 From: Aravind K P Date: Mon, 16 Aug 2021 12:13:11 +0530 Subject: [PATCH] cli: improve cli package error propogation in migrate operations https://github.com/hasura/graphql-engine-mono/pull/1887 GitOrigin-RevId: 40c7e1a16e8dd25a46e0c58afc7cbc9169af8f0b --- cli/commands/migrate_apply.go | 153 +++++++++++++--------- cli/internal/metadatautil/sources.go | 14 ++ cli/pkg/metadata/project_metadata_test.go | 2 +- cli/pkg/migrate/apply.go | 12 +- cli/pkg/migrate/project_migrate.go | 8 +- cli/pkg/migrate/project_migrate_test.go | 126 ++++++++++++++---- cli/pkg/migrate/status.go | 2 +- 7 files changed, 224 insertions(+), 93 deletions(-) diff --git a/cli/commands/migrate_apply.go b/cli/commands/migrate_apply.go index 82bff904411..36e5d676a91 100644 --- a/cli/commands/migrate_apply.go +++ b/cli/commands/migrate_apply.go @@ -109,6 +109,13 @@ func (o *MigrateApplyOptions) Validate() error { o.Source.Name = "" } + if o.DryRun && o.SkipExecution { + return errors.New("both --skip-execution and --dry-run flags cannot be used together") + } + if o.DryRun && o.AllDatabases { + return errors.New("both --all-databases and --dry-run flags cannot be used together") + } + if o.EC.Config.Version >= cli.V3 { if !o.AllDatabases && len(o.Source.Name) == 0 { return fmt.Errorf("unable to determine database on which migration should be applied") @@ -141,87 +148,111 @@ type errDatabaseMigrationDirectoryNotFound struct { func (e *errDatabaseMigrationDirectoryNotFound) Error() string { return e.message } + func (o *MigrateApplyOptions) Run() error { + results, err := o.Apply() + if err != nil { + return err + } + for result := range results { + if result.Error != nil { + o.EC.Logger.Errorf("%v", result.Error) + } else if len(result.Message) > 0 { + o.EC.Logger.Infof(result.Message) + } + } + return nil +} + +type MigrateApplyResult struct { + DatabaseName string + Message string + Error error +} + +func (o *MigrateApplyOptions) Apply() (chan MigrateApplyResult, error) { + resultChan := make(chan MigrateApplyResult) + + handleError := func(err error) (string, error) { + if err == migrate.ErrNoChange { + return fmt.Sprintf("nothing to apply on database %s", o.Source.Name), nil + } else if e, ok := err.(*os.PathError); ok { + // If Op is first, then log No migrations to apply + if e.Op == "first" { + return fmt.Sprintf("nothing to apply on database %s", o.Source.Name), nil + } + } else if e, ok := err.(*errDatabaseMigrationDirectoryNotFound); ok { + // check if the returned error is a directory not found error + // ie might be because a migrations/ directory is not found + // if so skip this + return "", fmt.Errorf("skipping applying migrations on database %s, encountered: \n%s", o.Source.Name, e.Error()) + } else if err != nil { + return "", fmt.Errorf("skipping applying migrations on database %s, encountered: \n%v", o.Source.Name, err) + } + return "", nil + } + if len(o.Source.Name) == 0 { o.Source = o.EC.Source } if err := o.Validate(); err != nil { - return err - } - if o.DryRun && o.SkipExecution { - return errors.New("both --skip-execution and --dry-run flags cannot be used together") + return nil, err } if o.AllDatabases && o.EC.Config.Version >= cli.V3 { o.EC.Spin("getting lists of databases from server ") sourcesAndKind, err := metadatautil.GetSourcesAndKind(o.EC.APIClient.V1Metadata.ExportMetadata) o.EC.Spinner.Stop() if err != nil { - return fmt.Errorf("determing list of connected sources and kind: %w", err) + return nil, err } - for _, source := range sourcesAndKind { - o.Source.Kind = source.Kind - o.Source.Name = source.Name + go func() { + defer close(resultChan) + for _, source := range sourcesAndKind { + result := MigrateApplyResult{ + DatabaseName: source.Name, + Message: "", + Error: nil, + } + o.Source.Kind = source.Kind + o.Source.Name = source.Name + if !o.DryRun { + o.EC.Spin(fmt.Sprintf("Applying migrations on database: %s ", o.Source.Name)) + } + err := o.Exec() + o.EC.Spinner.Stop() + if err != nil { + result.Message, result.Error = handleError(err) + } else { + result.Message = fmt.Sprintf("migrations applied on database: %s", o.Source.Name) + } + resultChan <- result + } + }() + } else { + go func() { + defer close(resultChan) if !o.DryRun { - o.EC.Spin(fmt.Sprintf("Applying migrations on database: %s ", o.Source.Name)) + o.EC.Spin("Applying migrations...") + } + result := MigrateApplyResult{ + DatabaseName: o.Source.Name, + Message: "", + Error: nil, } err := o.Exec() o.EC.Spinner.Stop() if err != nil { - if err == migrate.ErrNoChange { - o.EC.Logger.Infof("nothing to apply on database: %s", o.Source.Name) - continue - } - if e, ok := err.(*os.PathError); ok { - // If Op is first, then log No migrations to apply - if e.Op == "first" { - o.EC.Logger.Infof("nothing to apply on database: %s", o.Source.Name) - continue - } - } - // check if the returned error is a directory not found error - // ie might be because a migrations/ directory is not found - // if so skip this - if e, ok := err.(*errDatabaseMigrationDirectoryNotFound); ok { - o.EC.Logger.Errorf("skipping applying migrations for database %s, encountered: \n%s", o.Source.Name, e.Error()) - continue - } - o.EC.Logger.Errorf("skipping applying migrations for database %s, encountered: \n%v", o.Source.Name, err) - continue + result.Message, result.Error = handleError(err) + } else { + result.Message = "migrations applied" } - o.EC.Logger.Infof("applied migrations on database: %s", o.Source.Name) - } - } else { - if !o.DryRun { - o.EC.Spin("Applying migrations...") - } - err := o.Exec() - o.EC.Spinner.Stop() - if err != nil { - if err == migrate.ErrNoChange { - o.EC.Logger.Info("nothing to apply") - return nil - } - // check if the returned error is a directory not found error - // ie might be because a migrations/ directory is not found - // if so skip this - if e, ok := err.(*errDatabaseMigrationDirectoryNotFound); ok { - return fmt.Errorf("applying migrations on database %s: %w", o.Source.Name, e) - } - if e, ok := err.(*os.PathError); ok { - // If Op is first, then log No migrations to apply - if e.Op == "first" { - o.EC.Logger.Info("nothing to apply") - return nil - } - } - return fmt.Errorf("apply failed\n%w", err) - } - if !o.DryRun { - o.EC.Logger.Info("migrations applied") - } + resultChan <- result + }() } - return nil + + return resultChan, nil } + func (o *MigrateApplyOptions) Exec() error { if o.EC.Config.Version >= cli.V3 { // check if a migrations directory exists for source in project diff --git a/cli/internal/metadatautil/sources.go b/cli/internal/metadatautil/sources.go index 2f99dbec66c..2ad7f938ec1 100644 --- a/cli/internal/metadatautil/sources.go +++ b/cli/internal/metadatautil/sources.go @@ -110,6 +110,20 @@ func GetSourcesAndKind(exportMetadata func() (io.Reader, error)) ([]Source, erro return sources, nil } +var ErrNoConnectedSources = errors.New("0 connected sources found on hasura") + +// GetSourcesAndKindStrict is like GetSourcesAndKind but will return an error when no sources are found +func GetSourcesAndKindStrict(exportMetadata func() (io.Reader, error)) ([]Source, error) { + sources, err := GetSourcesAndKind(exportMetadata) + if err != nil { + return nil, err + } + if len(sources) == 0 { + return nil, ErrNoConnectedSources + } + return sources, nil +} + func DatabaseChooserUI(exportMetadata func() (io.Reader, error)) (string, error) { sources, err := GetSources(exportMetadata) if err != nil { diff --git a/cli/pkg/metadata/project_metadata_test.go b/cli/pkg/metadata/project_metadata_test.go index e4a3366421e..2e466c99c9c 100644 --- a/cli/pkg/metadata/project_metadata_test.go +++ b/cli/pkg/metadata/project_metadata_test.go @@ -220,7 +220,7 @@ func TestProjectMetadata_GetInconsistentMetadata(t *testing.T) { // - apply metadata // - drop a table via run_sql API // - reload metadata - err := migrations.Apply(migrate.ApplyOnAllDatabases()) + _, err := migrations.Apply(migrate.ApplyOnAllDatabases()) require.NoError(t, err) _, err = metadata.Apply() require.NoError(t, err) diff --git a/cli/pkg/migrate/apply.go b/cli/pkg/migrate/apply.go index 6d18242a3de..a07b09a0b37 100644 --- a/cli/pkg/migrate/apply.go +++ b/cli/pkg/migrate/apply.go @@ -41,9 +41,17 @@ func ApplyVersion(version string, direction MigrationDirection) ProjectMigration } } -func (p *projectMigrationsApplier) Apply(opts ...ProjectMigrationApplierOption) error { +func (p *projectMigrationsApplier) apply(opts ...ProjectMigrationApplierOption) ([]ApplyResult, error) { for _, opt := range opts { opt(p) } - return p.opts.Run() + var results []ApplyResult + resultChan, err := p.opts.Apply() + if err != nil { + return nil, err + } + for v := range resultChan { + results = append(results, ApplyResult(v)) + } + return results, nil } diff --git a/cli/pkg/migrate/project_migrate.go b/cli/pkg/migrate/project_migrate.go index a973c1aec58..c6e91440c1a 100644 --- a/cli/pkg/migrate/project_migrate.go +++ b/cli/pkg/migrate/project_migrate.go @@ -4,6 +4,8 @@ import ( "fmt" "io" + "github.com/hasura/graphql-engine/cli/v2/commands" + "github.com/hasura/graphql-engine/cli/v2" "github.com/spf13/viper" ) @@ -28,9 +30,11 @@ func (p *ProjectMigrate) StatusJSON(opts ...ProjectMigrationStatusOption) (io.Re return lister.StatusJSON(opts...) } -func (p *ProjectMigrate) Apply(opts ...ProjectMigrationApplierOption) error { +type ApplyResult commands.MigrateApplyResult + +func (p *ProjectMigrate) Apply(opts ...ProjectMigrationApplierOption) ([]ApplyResult, error) { applier := newProjectMigrationsApplier(p.ec) - return applier.Apply(opts...) + return applier.apply(opts...) } func NewProjectMigrate(projectDirectory string, opts ...ProjectMigrateOption) (*ProjectMigrate, error) { diff --git a/cli/pkg/migrate/project_migrate_test.go b/cli/pkg/migrate/project_migrate_test.go index 92e8d976cf0..28cb13f79d5 100644 --- a/cli/pkg/migrate/project_migrate_test.go +++ b/cli/pkg/migrate/project_migrate_test.go @@ -37,6 +37,7 @@ func TestProjectMigrate_ApplyConfig_v3(t *testing.T) { name string fields fields args args + want []ApplyResult wantErr bool }{ { @@ -48,6 +49,18 @@ func TestProjectMigrate_ApplyConfig_v3(t *testing.T) { args{ []ProjectMigrationApplierOption{ApplyOnAllDatabases()}, }, + []ApplyResult{ + { + "s1", + "migrations applied on database: s1", + nil, + }, + { + "s2", + "migrations applied on database: s2", + nil, + }, + }, false, }, { @@ -59,6 +72,13 @@ func TestProjectMigrate_ApplyConfig_v3(t *testing.T) { args{ []ProjectMigrationApplierOption{ApplyOnDatabaseName("s1"), ApplyVersion("1623841477474", MigrationDirectionDown)}, }, + []ApplyResult{ + { + "s1", + "migrations applied", + nil, + }, + }, false, }, { @@ -70,6 +90,13 @@ func TestProjectMigrate_ApplyConfig_v3(t *testing.T) { args{ []ProjectMigrationApplierOption{ApplyOnDatabaseName("s1"), ApplyVersion("1623841477474", MigrationDirectionUp)}, }, + []ApplyResult{ + { + "s1", + "migrations applied", + nil, + }, + }, false, }, } @@ -77,11 +104,12 @@ func TestProjectMigrate_ApplyConfig_v3(t *testing.T) { t.Run(tt.name, func(t *testing.T) { p, err := NewProjectMigrate(tt.fields.projectDirectory, WithAdminSecret(testutil.TestAdminSecret), WithEndpoint(tt.fields.endpointString)) require.NoError(t, err) - err = p.Apply(tt.args.opts...) + got, err := p.Apply(tt.args.opts...) if tt.wantErr { require.Error(t, err) } else { require.NoError(t, err) + require.Equal(t, tt.want, got) } }) } @@ -103,6 +131,7 @@ func TestProjectMigrate_Apply_Configv2(t *testing.T) { name string fields fields args args + want []ApplyResult wantErr bool }{ { @@ -115,6 +144,11 @@ func TestProjectMigrate_Apply_Configv2(t *testing.T) { args{ []ProjectMigrationApplierOption{ApplyOnAllDatabases()}, }, + []ApplyResult{ + { + Message: "migrations applied", + }, + }, false, }, { @@ -127,6 +161,11 @@ func TestProjectMigrate_Apply_Configv2(t *testing.T) { args{ []ProjectMigrationApplierOption{ApplyVersion("1623842054907", MigrationDirectionDown)}, }, + []ApplyResult{ + { + Message: "migrations applied", + }, + }, false, }, { @@ -139,7 +178,12 @@ func TestProjectMigrate_Apply_Configv2(t *testing.T) { args{ []ProjectMigrationApplierOption{ApplyVersion("1623842054907", MigrationDirectionDown)}, }, - true, + []ApplyResult{ + { + Error: fmt.Errorf("skipping applying migrations on database , encountered: \nMigration not applied in database"), + }, + }, + false, }, { "can apply up migrations of a version on a config v2 project", @@ -151,6 +195,11 @@ func TestProjectMigrate_Apply_Configv2(t *testing.T) { args{ []ProjectMigrationApplierOption{ApplyVersion("1623842054907", MigrationDirectionUp)}, }, + []ApplyResult{ + { + Message: "migrations applied", + }, + }, false, }, } @@ -158,11 +207,12 @@ func TestProjectMigrate_Apply_Configv2(t *testing.T) { t.Run(tt.name, func(t *testing.T) { p, err := NewProjectMigrate(tt.fields.projectDirectory, WithAdminSecret(testutil.TestAdminSecret), WithEndpoint(tt.fields.endpointString)) require.NoError(t, err) - err = p.Apply(tt.args.opts...) + got, err := p.Apply(tt.args.opts...) if tt.wantErr { require.Error(t, err) } else { require.NoError(t, err) + require.Equal(t, tt.want, got) } }) } @@ -287,7 +337,8 @@ func TestProjectMigrate_Status_ConfigV2(t *testing.T) { ]`, false, func(t *testing.T, p *ProjectMigrate) { - assert.NoError(t, p.Apply(ApplyOnAllDatabases())) + _, err := p.Apply(ApplyOnAllDatabases()) + assert.NoError(t, err) }, }, } @@ -332,25 +383,24 @@ func TestProjectMigrate_Status_ConfigV3(t *testing.T) { type fields struct { projectDirectory string adminSecret string - endpointString string } type args struct { opts []ProjectMigrationStatusOption } tests := []struct { - name string - fields fields - args args - want string - wantErr bool - before func(t *testing.T, p *ProjectMigrate) + name string + fields fields + args args + want string + wantErr bool + testSetup func() (hgeEndpoint string, teardown func()) + before func(t *testing.T, p *ProjectMigrate) }{ { "can get status of migrations", fields{ projectDirectory: "testdata/projectv3", adminSecret: "", - endpointString: hgeEndpoint, }, args{ opts: []ProjectMigrationStatusOption{}, @@ -426,6 +476,7 @@ func TestProjectMigrate_Status_ConfigV3(t *testing.T) { } ]`, false, + func() (string, func()) { return hgeEndpoint, func() {} }, func(t *testing.T, p *ProjectMigrate) {}, }, { @@ -433,7 +484,6 @@ func TestProjectMigrate_Status_ConfigV3(t *testing.T) { fields{ projectDirectory: "testdata/projectv3", adminSecret: "", - endpointString: hgeEndpoint, }, args{ opts: []ProjectMigrationStatusOption{}, @@ -510,32 +560,56 @@ func TestProjectMigrate_Status_ConfigV3(t *testing.T) { } ]`, false, + func() (string, func()) { return hgeEndpoint, func() {} }, func(t *testing.T, p *ProjectMigrate) { - assert.NoError(t, p.Apply(ApplyOnAllDatabases())) + _, err := p.Apply(ApplyOnAllDatabases()) + assert.NoError(t, err) + }, + }, + { + "can throw an error when no databases are connected to hge", + fields{ + projectDirectory: "testdata/projectv3", + adminSecret: "", + }, + args{ + opts: []ProjectMigrationStatusOption{}, + }, + ``, + true, + func() (string, func()) { + port, teardown := testutil.StartHasuraWithMetadataDatabase(t, testutil.HasuraDockerImage) + return fmt.Sprintf("http://%s:%s", testutil.Hostname, port), teardown + }, + func(t *testing.T, p *ProjectMigrate) { + _, err := p.Apply(ApplyOnAllDatabases()) + assert.NoError(t, err) }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - p, err := NewProjectMigrate(tt.fields.projectDirectory, WithAdminSecret(testutil.TestAdminSecret), WithEndpoint(tt.fields.endpointString)) + hgeEndpoint, setupTeardown := tt.testSetup() + defer setupTeardown() + p, err := NewProjectMigrate(tt.fields.projectDirectory, WithAdminSecret(testutil.TestAdminSecret), WithEndpoint(hgeEndpoint)) require.NoError(t, err) - applier, err := NewProjectMigrate(tt.fields.projectDirectory, WithAdminSecret(testutil.TestAdminSecret), WithEndpoint(tt.fields.endpointString)) + applier, err := NewProjectMigrate(tt.fields.projectDirectory, WithAdminSecret(testutil.TestAdminSecret), WithEndpoint(hgeEndpoint)) require.NoError(t, err) tt.before(t, applier) got, err := p.status(tt.args.opts...) if tt.wantErr { require.Error(t, err) - } - require.NoError(t, err) - gotJSON, err := json.Marshal(got) - require.NoError(t, err) - require.JSONEq(t, tt.want, string(gotJSON)) + } else { + gotJSON, err := json.Marshal(got) + require.NoError(t, err) + require.JSONEq(t, tt.want, string(gotJSON)) - statusJson, err := p.StatusJSON(tt.args.opts...) - require.NoError(t, err) - statusJsonb, err := ioutil.ReadAll(statusJson) - require.NoError(t, err) - require.JSONEq(t, tt.want, string(statusJsonb)) + statusJson, err := p.StatusJSON(tt.args.opts...) + require.NoError(t, err) + statusJsonb, err := ioutil.ReadAll(statusJson) + require.NoError(t, err) + require.JSONEq(t, tt.want, string(statusJsonb)) + } }) } } diff --git a/cli/pkg/migrate/status.go b/cli/pkg/migrate/status.go index 3ad86d45226..c551a92fbe7 100644 --- a/cli/pkg/migrate/status.go +++ b/cli/pkg/migrate/status.go @@ -29,7 +29,7 @@ func (p *projectMigrationsStatus) Status(opts ...ProjectMigrationStatusOption) ( } if p.allDatabases { metadataOps := cli.GetCommonMetadataOps(p.ec) - sources, err := metadatautil.GetSourcesAndKind(metadataOps.ExportMetadata) + sources, err := metadatautil.GetSourcesAndKindStrict(metadataOps.ExportMetadata) if err != nil { return nil, err }