Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix: init script sed in non existent file #1246

Merged
merged 2 commits into from
Mar 16, 2018

Conversation

JGAntunes
Copy link
Member

Running:

$ docker run -it -p 4002:4002 -p 4003:4003 -p 5002:5002 -p 9090:9090 ipfs/js-ipfs:latest

Is returning:

Using /root/.jsipfs as IPFS repository
sed: can't read /root/.jsipfs/config: No such file or directory

Seems like the sed command in the init-and-daemon.sh script is performed against a non existing file since the repo might not exist (which is the case in a brand new container without any volume attached).

I went ahead and changed the order of the commands (it works like this). I assume that this was the intended behaviour, but might be missing some reason as to why this should be arranged in a different form. If anyone wants to test it out - https://hub.docker.com/r/jgantunes/js-ipfs/

@JGAntunes JGAntunes requested a review from victorb March 5, 2018 03:30
@ghost ghost assigned JGAntunes Mar 5, 2018
@ghost ghost added the status/in-progress In progress label Mar 5, 2018
@victorb
Copy link
Member

victorb commented Mar 5, 2018

We probably shoud do it like ipfs-cluster does it: https://github.com/ipfs/ipfs-cluster/blob/master/docker/entrypoint.sh#L24-L26

Have a environment variable where you can set your own value, and use default 127.0.0.1 if it doesn't exists. As it is in this PR (and before), 0.0.0.0 would indicate bind on all interfaces, which might not always be desireble.

@daviddias
Copy link
Member

@victorbjelkholm I agree with your point, nevertheless seems that this is fixing a bug. Anything against merging @JGAntunes PR?

@victorb
Copy link
Member

victorb commented Mar 6, 2018

It's indeed fixing a bug but as my previous comment indicates, I don't think it's the right fix. If you're looking for a workaround while we have a proper fix, there is images published under https://hub.docker.com/r/jgantunes/js-ipfs/

@JGAntunes
Copy link
Member Author

Hey @victorbjelkholm @diasdavid !

I don't mind changing this to use env vars honestly. Would the expected behaviour be the same as the ipfs-cluster example? When the env var exists we use it else we keep the config as it is and bind to the loopback interface? And the docker image sets a value by default to 0.0.0.0?

@victorb
Copy link
Member

victorb commented Mar 6, 2018

@JGAntunes thanks 💯

Yeah, if the env var is not set, don't do anything (js-ipfs defaults to using 127.0.0.1 by itself, which is correct) but if the env var is set, replace it with the value from the env var. (it should not default to 0.0.0.0 which we currently have). If you want to set the value to 0.0.0.0 you would have to use the env var for it.

@JGAntunes
Copy link
Member Author

Sorry for the delay guys. @victorbjelkholm is this what you were expecting? I named it IPFS_API_HOST, let me know if that works for you.

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

LGTM

@daviddias daviddias merged commit 75d47c3 into ipfs:master Mar 16, 2018
@ghost ghost removed the status/in-progress In progress label Mar 16, 2018
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.

3 participants