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

fix: do not cleanup api and repo.lock files #711

Merged
merged 2 commits into from
Nov 24, 2018
Merged

fix: do not cleanup api and repo.lock files #711

merged 2 commits into from
Nov 24, 2018

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Nov 24, 2018

Previously we were cleaning up the api and repo.lock files because the shutdown on Windows was not working correctly on the IPFS daemon. It seems the issue was already resolved (see ipfs/kubo#3061).

This PR removes that code and this should also fix #689 and fix #688.

@lidel can you check this out, please?

@hacdias hacdias requested a review from lidel November 24, 2018 10:36
@ghost ghost assigned hacdias Nov 24, 2018
@ghost ghost added the in progress label Nov 24, 2018
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented Nov 24, 2018

I also removed our path definition and now it uses ipfsd-ctl default path which is the IPFS_PATH variable if present or $HOME/.ipfs otherwise.

@lidel we now have a small issue: if the repo has a api file and doesn't have a repo.lock file, js-ipfsd-ctl will try to connect to the API and will fail if the daemon isn't running. Do you think we should catch the ECONNREFUSED error and remove the api file?

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Works as expected and improves things a lot for me, thanks! 👍
(I was able to use external node from Docker)

Do you think we should catch the ECONNREFUSED error and remove the api file?

I strongly believe in "no surprises" when it comes to desktop apps, so we should aim at not removing anything from $IPFS_PATH/ without explicit consent from user.

For now we should just detect ECONNREFUSED and display human-readable error window that remote node defined in $IPFS_PATH/api is offline and add "Retry" and "Quit" buttons (leaving it up to technical user to start node or edit the address in file).

In the long run (future PR), we probably want to make this built-in and super nice, wrote some ideas in: #713

@hacdias
Copy link
Member Author

hacdias commented Nov 24, 2018

Thanks @lidel. I'll create the dialog on a different PR!

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.

Fails to start when ~/.ipfs/api exists Missing support for external daemon already running
2 participants