Skip to content

fix(migrators/goose): read from filesytem if no FS #26

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

Merged
merged 1 commit into from
May 25, 2025

Conversation

dmksnnk
Copy link
Contributor

@dmksnnk dmksnnk commented Jan 3, 2025

This CR should allow to read migrations folder from parent directory like ../migrations.

I have a migrations directory in the root of the project, so to make migrations work, I need to provide FS with root directory:

root, err := filepath.Abs("../../../..")
if err != nil {
  t.Fatal("get abs path", err)
}

migrator := goosemigrator.New("migrations", goosemigrator.WithFS(os.DirFS(root)))

This CR should allow to do just:

migrator := goosemigrator.New("../../../../migrations")

Also, golangmigrator does not have fs.FS by default too.

@peterldowns
Copy link
Owner

Seems reasonable! I think the tests can be simplified, so doing that in this PR #33

@peterldowns peterldowns closed this May 6, 2025
@peterldowns peterldowns reopened this May 6, 2025
@peterldowns
Copy link
Owner

@dmksnnk ah you know what, the nil FS seems to cause a panic inside goose. If you can get the tests passing, and update the tests to look more like what I tried in #33 (one case with migrations, one case with ../goosemigrator/migrations, same logic copy-pasted between them instead of extracting a function and using os.chdir) I'd be willing to merge.

@peterldowns peterldowns added the feature feature request label May 6, 2025
@dmksnnk dmksnnk force-pushed the fix/goose-parent-dir branch from 5fed2a2 to 014604b Compare May 20, 2025 23:12
@dmksnnk
Copy link
Contributor Author

dmksnnk commented May 20, 2025

Hi @peterldowns, should be fixed, please take a look 🙂

Copy link
Owner

@peterldowns peterldowns left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@peterldowns peterldowns merged commit 7792810 into peterldowns:main May 25, 2025
3 checks passed
@peterldowns
Copy link
Owner

I'll tag a release later today. Thanks for the contribution @dmksnnk :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants