-
-
Notifications
You must be signed in to change notification settings - Fork 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
Upgrade graphsync deps #7598
Upgrade graphsync deps #7598
Conversation
) | ||
|
||
replace github.com/ipfs/go-ipfs => ./../../.. |
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.
Interesting. Obviously we need this for now (i.e. until the go-ipfs 0.7.0 release) in order to get things to work, but should we just keep this replace directive here since it's an example?
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.
In e.g. go-car it is permanent: https://github.com/ipld/go-car/blob/v0.1.0/car/go.mod#L11
I don't mind either way, we can even remove it right now... ( there are no tests against it )
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.
Deps look upgraded and tests are passing so LGTM 😄. What's the status of a go-car release?
@@ -55,7 +55,7 @@ require ( | |||
github.com/ipfs/go-unixfs v0.2.4 | |||
github.com/ipfs/go-verifcid v0.0.1 | |||
github.com/ipfs/interface-go-ipfs-core v0.4.0 | |||
github.com/ipld/go-car v0.1.0 | |||
github.com/ipld/go-car v0.1.1-0.20200429200904-c222d793c339 |
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.
Is there a release of go-car we can use 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.
No, this is what I was referring to in my original comment: go-car needs a bit more work for a release.
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.
@aschmahmann this is go-car master fwiw, so you are not depending on some random commit. I need to sneak in a particular fix in ( no API changes ), and that's another day or 2 away.
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.
So should I be merging this and then we can update go-car later or am I waiting to merge on a go-car release?
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.
Let's merge this now, I won't be ready in a reasonable timeframe.
There was supposed to be a bit more work landing here, but I won't get to it in time this week.
Let's unblock ipfs/go-graphsync#82 for now, maybe will have the more-involved go-car fix for rc2