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

sql/sqltool: wrap flyway dir with fs #1642

Merged
merged 12 commits into from
May 10, 2023
Merged

sql/sqltool: wrap flyway dir with fs #1642

merged 12 commits into from
May 10, 2023

Conversation

datdao
Copy link
Member

@datdao datdao commented May 9, 2023

Extending FlywayDir Functionality with fs.FS

By wrapping FlywayDir with fs.FS, we can extend its functionality to include local directories and directories in memory or remote directories.

@giautm giautm self-assigned this May 9, 2023
@giautm giautm requested review from masseelch and giautm May 9, 2023 17:26
sql/sqltool/tool.go Outdated Show resolved Hide resolved
sql/sqltool/tool_test.go Outdated Show resolved Hide resolved
@datdao datdao requested a review from giautm May 10, 2023 02:03
Copy link
Member

@giautm giautm left a comment

Choose a reason for hiding this comment

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

LGTM, @masseelch wdyt?

sql/sqltool/tool.go Outdated Show resolved Hide resolved
Co-authored-by: Ariel Mashraki <7413593+a8m@users.noreply.github.com>
Comment on lines 294 to 296
// FlywayDir wraps migrate.LocalDir and provides a migrate.Scanner implementation able to understand files
// generated by the FlywayFormatter for migration directory replaying.
FlywayDir struct{ *migrate.LocalDir }
FlywayDir struct{ fs.FS }
Copy link
Member

Choose a reason for hiding this comment

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

Edit comment as well

sql/sqltool/tool.go Outdated Show resolved Hide resolved
sql/sqltool/tool.go Outdated Show resolved Hide resolved
Copy link
Member

@masseelch masseelch left a comment

Choose a reason for hiding this comment

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

Like the idea. However, I suggest to wrap migrate.Dir instead of fs.FS.

@giautm
Copy link
Member

giautm commented May 10, 2023

Like the idea. However, I suggest to wrap migrate.Dir instead of fs.FS.

Because migrate.Dir has more methods than fs.FS so we can't use FlywayDir with os.DirFS or other file-system. The target of this PR is able to use with fs.FS.

sql/sqltool/tool.go Outdated Show resolved Hide resolved
sql/sqltool/tool.go Outdated Show resolved Hide resolved
@datdao datdao requested a review from masseelch May 10, 2023 13:24
sql/sqltool/tool.go Outdated Show resolved Hide resolved
Co-authored-by: Jannik Clausen <12862103+masseelch@users.noreply.github.com>
@giautm giautm merged commit 1c8d513 into ariga:master May 10, 2023
24 checks passed
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.

None yet

5 participants