Skip to content
This repository has been archived by the owner on Aug 12, 2022. It is now read-only.

feat: Testable Migrations (SDK) #146

Merged
merged 59 commits into from
Jan 18, 2022
Merged

feat: Testable Migrations (SDK) #146

merged 59 commits into from
Jan 18, 2022

Conversation

disq
Copy link
Member

@disq disq commented Jan 10, 2022

Moving migrations only to go-migrate, providing tools to generate SQLs (full and incremental) from schema.Table
Moving protocol version to 4

Please see relevant PRs on other projects:
cloudquery/cq-provider-aws#401
cloudquery/cloudquery#406
https://github.com/cloudquery/docs/pull/75
https://github.com/cloudquery/cq-provider-template/pull/12

@disq disq requested a review from roneli January 10, 2022 23:46
@disq disq changed the title Testable Migrations Testable Migrations (SDK) Jan 11, 2022
@disq disq changed the title Testable Migrations (SDK) feat: Testable Migrations (SDK) Jan 11, 2022
@disq disq marked this pull request as ready for review January 14, 2022 09:29
Copy link
Contributor

@roneli roneli left a comment

Choose a reason for hiding this comment

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

More comments, looking good! we just need to solve some issues around the comments and where QueryExecer interface should exist.

migration/migration.go Show resolved Hide resolved
migration/table.go Outdated Show resolved Hide resolved
migration/cli.go Show resolved Hide resolved
migration/migrator/migrator.go Show resolved Hide resolved
Comment on lines 29 to 43
type Database interface {
QueryExecer

Insert(ctx context.Context, t *Table, instance Resources) error
Delete(ctx context.Context, t *Table, kvFilters []interface{}) error
RemoveStaleData(ctx context.Context, t *Table, executionStart time.Time, kvFilters []interface{}) error
CopyFrom(ctx context.Context, resources Resources, shouldCascade bool, CascadeDeleteFilters map[string]interface{}) error
Close()
Dialect() Dialect
}

type QueryExecer interface {
Exec(ctx context.Context, query string, args ...interface{}) error
Query(ctx context.Context, query string, args ...interface{}) (pgx.Rows, error)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, I think my comments confused a little bit. Would move QueryExecer -> database package (as like you said core packages use it and its a database thing). The Database interface is only used by execution as its an execution thing, and then I would rename it perhaps.

provider/schema/execution.go Outdated Show resolved Hide resolved
@disq disq requested a review from roneli January 17, 2022 14:13
Copy link
Contributor

@roneli roneli left a comment

Choose a reason for hiding this comment

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

More comments, mostly nitpicks. only one logical one regarding drop/create diffs

helpers/dsn.go Show resolved Hide resolved
database/database.go Show resolved Hide resolved
database/database.go Show resolved Hide resolved
database/dialect.go Outdated Show resolved Hide resolved
migration/table.go Outdated Show resolved Hide resolved
migration/table.go Show resolved Hide resolved
migration/testbuilder.go Outdated Show resolved Hide resolved
provider/schema/dialect.go Show resolved Hide resolved
provider/schema/execution.go Show resolved Hide resolved
@disq disq requested a review from roneli January 17, 2022 21:50
Copy link
Contributor

@roneli roneli left a comment

Choose a reason for hiding this comment

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

LGTM, minor nits left.

database/dsn/dsn.go Show resolved Hide resolved
database/dsn/dsn.go Show resolved Hide resolved
migration/migrator/migrator.go Outdated Show resolved Hide resolved
migration/testbuilder.go Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants