Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

fix: revert back to go-ipfs-ds-help@v0.1.1 #92

Merged
merged 1 commit into from
Nov 18, 2021
Merged

Conversation

guseggert
Copy link
Contributor

This was inadvertently upgraded to v1

blockstore.go Show resolved Hide resolved
@@ -6,7 +6,7 @@ require (
github.com/ipfs/go-block-format v0.0.2
github.com/ipfs/go-cid v0.0.5
github.com/ipfs/go-datastore v0.5.0
github.com/ipfs/go-ipfs-ds-help v1.1.0
github.com/ipfs/go-ipfs-ds-help v0.1.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions:

  1. Are we intentionally skipping updating this repo's go-datastore dependency because it doesn't use contexts?
  2. How did you decide which dependencies to update here? go-cid and go-multihash got bumped in addition to go-datastore.
    • Should we be updating more, less, or exactly the number of dependencies we have here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I'm not sure what you mean? go-datastore@v0.5.0 is a new version w/ the context changes (ipfs/go-datastore@b23ab2f)

  2. In general I try to run go get github.com/ipfs/<repo>@<version> and then go mod tidy and respect whatever the result is. I'm supposing that Go is changing some seemingly-unrelated dependencies in line with its MVS algorithm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I meant that go-ipfs-ds-help has a dependency on go-datastore but wasn't updated. It doesn't seem to be a huge deal since that repo is just working with datastore.Key rather than datastore.Datastore (or related interfaces). Are we intentionally not updating that one?
  2. In this case did you do go get github.com/ipfs/go-datastore@v0.5.0? It looks like the original version of this (feat: add context to interfaces & plumb through datastore contexts #89) pulled in extra upgrades here. The updates seem fine, I mostly want to make sure I understand when and why we're updating things (e.g. we updated go-cid from v0.0.4 to v0.0.5 although it's not the latest) so it's easier to review + merge the follow up PRs.

Copy link
Contributor Author

@guseggert guseggert Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Ah right, none of the code that go-ipfs-ds-help uses from go-datastore is affected by the changes, so the build still succeeds. It's not strictly necessary here, but it will make MVS confusing down the line so it's a good idea to upgrade it anyway. Will do.
  2. Yeah that's what I did...generally I think what happens is that, ex. upgrading module A transitively changes the version of module B, and then when Go does MVS, the version of B is higher (but not necessarily the highest due to MVS), and then Go writes those into go.mod as well. There are additional rules about what goes into go.mod based on the version of Go you're using, and also the version of Go version that is specified in go.mod.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. k, lmk if you need more reviews + stamps (Slack is generally a faster ping then GitHub for this crazy amount of bubbling).
  2. It's no big deal, I'm not worried about the version bumps here. I think in this case they may have come from duplicating commits between the v1 and v0 branches here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually for 1) we should be good without changing anything, MVS should work fine.

This was inadvertently upgraded to v1
@guseggert guseggert merged commit e2c1e54 into release/v0 Nov 18, 2021
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants