Skip to content
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

chore: update ipfs-repo #241

Closed
wants to merge 1 commit into from
Closed

chore: update ipfs-repo #241

wants to merge 1 commit into from

Conversation

hugomrdias
Copy link
Member

Needs this PR linked ipfs/js-ipfs#1335

vasco-santos
vasco-santos previously approved these changes May 8, 2018
@vasco-santos
Copy link
Member

vasco-santos commented May 8, 2018

@diasdavid should I be able to merge PRs for js-ipfsd-ctl? I have no permissions for that.

@victorb
Copy link
Member

victorb commented May 8, 2018

@vasco-santos as a lead maintainer, yes. Added write permissions for you to this repository now.

@dryajov dryajov mentioned this pull request May 9, 2018
@daviddias
Copy link
Member

@vasco-santos weird, you should have had write permissions all the time given that you are in the JS team

image

package.json Outdated
@@ -84,7 +84,7 @@
"hapi": "^16.6.2",
"hat": "0.0.3",
"ipfs-api": "^20.0.1",
"ipfs-repo": "^0.19.0",
"ipfs-repo": "~0.21.0",
Copy link
Member

Choose a reason for hiding this comment

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

this repo version causes issues with latest js-ipfs (until the CLI errors PR is merged) //cc @hugomrdias

Copy link
Member

Choose a reason for hiding this comment

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

This PR is blocked until ipfs/js-ipfs#1335 is done given ipfs/js-ipfs-repo#165

package.json Outdated
@@ -84,7 +84,7 @@
"hapi": "^16.6.2",
"hat": "0.0.3",
"ipfs-api": "^20.0.1",
"ipfs-repo": "^0.19.0",
"ipfs-repo": "~0.21.0",
Copy link
Member

Choose a reason for hiding this comment

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

This PR is blocked until ipfs/js-ipfs#1335 is done given ipfs/js-ipfs-repo#165

@dryajov
Copy link
Member

dryajov commented May 14, 2018

@vasco-santos @hugomrdias @victorbjelkholm I believe that we need to pin the ipfs-repo version to 0.20.0, since anything above that will break due to the changes in which repo errors are being handled in newer versions.

@vasco-santos
Copy link
Member

I think that we need to release the new versions of ipfsd-ctl and ipfs (with the CLI errors fixing branch), in order to get everything green again. With ipfs-repo@0.20.0, I believe that the @hugomrdias 's CLI errors branch will not work.

@dryajov
Copy link
Member

dryajov commented May 14, 2018

I think that we need to release the new versions of ipfsd-ctl and ipfs (with the CLI errors fixing branch), in order to get everything green again.

Sure, but ipfsd-ctl is most likely broken for everybody right now due to the use of ^ instead of ~ for ipfs-repo version.

@daviddias
Copy link
Member

See

42597c1

and IRC log

21:00 <@daviddias> dryajov: why does ipfsd-ctl need to import ipfs-repo?
21:00 <@daviddias> What I see is that it grabs a path and creates a new IPFSRepo rather than passing that path to js-ipfs itself
21:01 <dryajov> trying to remember what was the reason for that now…
21:01 <dryajov> let me think
21:01 → jesse22 joined  ⇐ jesse22_ quit  
21:03 <@daviddias> testing with it removed, works
21:03 <@daviddias> it is used by tests
21:04 <@daviddias> let me see if tests also need it
21:04 <@daviddias> removing
21:04 <@daviddias> *moving
21:05 <dryajov> I think it was done so we can generate random repo names...
21:06 <dryajov> but we can just generate the path, without having to use the repo...
21:06 <@daviddias> exactly

ipfs-repo is not needed by this module at all :)

@daviddias daviddias closed this May 14, 2018
@ghost ghost removed the status/in-progress In progress label May 14, 2018
@daviddias daviddias deleted the chore/update-ipfs-repo branch May 14, 2018 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants