Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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

Remove use of filepath for interacting with embedded migration files #485

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Jul 31, 2024

This one is in pursuit of resolving #482 in which migrations can't
currently run on Windows.

As @bgentry points out regarding the embed package [1], where forward
slashes are used for all files regardless of underlying operation system:

The patterns are interpreted relative to the package directory
containing the source file. The path separator is a forward slash, even
on Windows systems.

But filepath uses OS-specific separators, making commingling use of
these two packages not advisable:

The filepath package uses either forward slashes or backslashes,
depending on the operating system. To process paths such as URLs that
always use forward slashes regardless of the operating system, see the
path package.

Here, remove use of filepath for migrations by using the IsDir
function of a file entry instead, which also turns out to be much
cleaner anyway.

I don't have a Windows system to test on, but this seems like a pretty
likely fix. A Windows-specific job to test the CLI is in progress as
part of #483, which would help prevent future regressions.

Fixes #482.

[1] https://pkg.go.dev/embed#hdr-Directives

@bgentry
Copy link
Contributor

bgentry commented Jul 31, 2024

I just set up Windows CI for the CLI in #483, which reveals the exact issue this PR is supposed to fix. Let's merge that and rebase this one on top of it to confirm the fix?

@bgentry
Copy link
Contributor

bgentry commented Jul 31, 2024

Unfortunately this does not appear to resolve the issue. I rebased my PR #483 on this one and did a single run, and a similar panic happened: https://github.com/riverqueue/river/actions/runs/10172817143/job/28135934181?pr=483

However, with the CI in that branch we can at least reproduce this somewhat easily. We can merge that and then you can push a branch with some debug output to help track this down. Not ideal, but neither is finding a real Windows machine to test on 😉

@brandur brandur force-pushed the brandur-remove-filepath branch 4 times, most recently from 4199794 to 4bf5072 Compare July 31, 2024 14:53
This one is in pursuit of resolving #482 in which migrations can't
currently run on Windows.

As @bgentry points out regarding the `embed` package [1], where forward
slashes are used for all files regardless of underlying operation system:

> The patterns are interpreted relative to the package directory
> containing the source file. The path separator is a forward slash, even
> on Windows systems.

But `filepath` uses OS-specific separators, making commingling use of
these two packages not advisable:

> The filepath package uses either forward slashes or backslashes,
> depending on the operating system. To process paths such as URLs that
> always use forward slashes regardless of the operating system, see the
> path package.

Here, remove use of `filepath` for migrations by using the `IsDir`
function of a file entry instead, which also turns out to be much
cleaner anyway.

I don't have a Windows system to test on, but this seems like a pretty
likely fix. A Windows-specific job to test the CLI is in progress as
part of #483, which would help prevent future regressions.

Fixes #482.

[1] https://pkg.go.dev/embed#hdr-Directives
@brandur
Copy link
Contributor Author

brandur commented Jul 31, 2024

@bgentry So I tried this against your Window job, and it's a little unintuitive, but it actually does work. The problem is that along with this change you also need to uncomment the replace directives in cmd/river/go.mod so that the CLI there pulls in the version of rivermigrate that's on this branch (otherwise it's using the latest tagged version).

As we're sending out this fix we'll need to make sure to cut a new CLI release with bumped versions of the other River packages.

@bgentry
Copy link
Contributor

bgentry commented Jul 31, 2024

@brandur sweet! This will just need a rebase to get the updated branch protection rules.

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.

Panic when using cli migration
2 participants