fix: unknown fetcher type error #8830
Conversation
lidel
force-pushed
the
fix/error-on-nil-fetcher
branch
from
4ff7f93
to
a6d0418
Compare
cmd/ipfs/add_migrations.go
Outdated
| case nil: | ||
| // https://github.com/ipfs/go-ipfs/issues/8780 | ||
| continue |
There was a problem hiding this comment.
How do we end up with one of these?
There was a problem hiding this comment.
We probably don't – opened this draft PR to run CI and then look closer.
I now suspect we don't have nil here, but a RetryFetcher which wraps NewHttpFetcher:
https://github.com/ipfs/go-ipfs/blob/a6d04184062d1883f090d9661a4d8170a726cfc4/repo/fsrepo/migrations/migrations.go#L165
iiuc switch over fetcher type interprets RetryFetcher as something other than IpfsFetcher and HttpFetcher and that is why default gets executed.
Quick demo: https://go.dev/play/p/MEKo40x1z0I
There was a problem hiding this comment.
@aschmahmann if we added RetryFetcher some time later, that probably introduced the bug.
Since it is only used for HTTP, we could fix it this way (https://go.dev/play/p/CTeMTLaEvN6):
- case *migrations.HttpFetcher:
+ case *migrations.HttpFetcher, *migrations.RetryFetcher:I did that in be916e7 – thoughts?
There was a problem hiding this comment.
Can we add something to the Fetcher interface for this so we can remove this brittle type switch altogether?
There was a problem hiding this comment.
Probably, but at the same time we won't be adding any new Fetcher types any time soon.
I'm merging this one-liner fix for now – fwiw we will be revisiting fetchers after we have car export on gateways, to fetch CAR instead of trusting ipfs.io to return right data, we can improve things as part of that work – filled #8851 for tracking that.
lidel
force-pushed
the
fix/error-on-nil-fetcher
branch
from
be916e7
to
a763f6c
Compare
Closes #8780