-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
updating to new datastore/blockstore code with contexts #7646
Conversation
3498145
to
c0fa25c
Compare
i'll work on finishing this as we need it to upgrade go-libp2p. |
the go-data-transfer dep is broken:
|
@aarshkshah1992 @dirkmc is the go-data-transfer stuff done? |
this is getting blocked by which is blocked by which is blocked by a release tag of |
@vyzo go-data-transfer thing should be fixed now |
broken go-ipfs-blockstore:
|
v1.1.1 also broken:
|
This occurs when trying to upgrade go-data-transfer:
|
seems it was a transient go mod cache error of some sorts. |
0c4f9fd
to
a9e22df
Compare
rebased and fixed deps; now the 1M places to add contexts... |
Blocker in go-fil-markets:
|
Blocker in go-car:
|
Blocker in go-ds-versioning
|
blocker in go-storedcounter:
|
blocker in drand:
|
a9e22df
to
6ba7481
Compare
go-storedcounter should be unblocked |
This is now ready to be reviewed and merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass, shouldn't be hard to land.
Was the ffi update intentional?
@@ -107,6 +108,7 @@ type Meta struct { | |||
// CreateImport initializes a new import, returning its ID and optionally a | |||
// CAR path where to place the data, if requested. | |||
func (m *Manager) CreateImport() (id ID, err error) { | |||
ctx := context.TODO() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably open a tracking issue about this (in markets?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what it should say and where it should be; what's the problem here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC This Manager
struct implements some interface in markets, it would be nice to have contexts plumbed, but I don't think it needs to happen right now - so we could just open an issue saying 'add contexts to this stuff'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened #7813.
probably accident, that was not me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one nitpick.
Also skimming through the dependency diff quickly (it's.. 30k lines)
(also need to merge master into here to sort out conflicts) |
63be845
to
bc384c0
Compare
… needs more testing before landing remove ctx datastore from autobatch testplans go mod tidy fix ctx
… needs more testing before landing
Status
The following deps need to be tagged, as we have go mod versions pointing at unstable things:
The following deps also exist in the lotus-soup go.mod: