-
Notifications
You must be signed in to change notification settings - Fork 73
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
base: master
Are you sure you want to change the base?
Conversation
b32ceb5
to
25391be
Compare
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? |
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 😉 |
4199794
to
4bf5072
Compare
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
4bf5072
to
afe8c66
Compare
@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 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. |
@brandur sweet! This will just need a rebase to get the updated branch protection rules. |
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 forwardslashes are used for all files regardless of underlying operation system:
But
filepath
uses OS-specific separators, making commingling use ofthese two packages not advisable:
Here, remove use of
filepath
for migrations by using theIsDir
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