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

update link to filestore experimental status #4557

Merged
merged 1 commit into from
Feb 14, 2018

Conversation

come-maiz
Copy link
Contributor

According to #3397 (comment), the status of experimental features is now tracked in a file in the repo, not in an issue.

According to ipfs#3397 (comment), the status of experimental features is now tracked in a file in the repo, not in an issue.

License: MIT
Signed-off-by: Leo Arias <[email protected]>
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM (links verified)

@@ -195,7 +195,7 @@ You can now check what blocks have been created by:
hashFunStr, hfset, _ := req.Option(hashOptionName).String()

if nocopy && !cfg.Experimental.FilestoreEnabled {
res.SetError(errors.New("filestore is not enabled, see https://git.io/vy4XN"),
res.SetError(errors.New("filestore is not enabled, see https://git.io/vNItf"),
cmdkit.ErrClient)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a personal dislike for the use of short links unless absolutely necessary and would prefer https://github.com/ipfs/go-ipfs/blob/master/docs/experimental-features.md#ipfs-filestore is used.

But since a short URL was already used I am not going to block this.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is because it happens in an error message. Tools have a bad habit of truncating logs/error messages so this is safer...

Copy link
Member

Choose a reason for hiding this comment

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

How about changing the error to print what the user has to do to enable it + the link to the doc? Something like:

Filestore is not enabled. Run `ipfs config --json Experimental.FilestoreEnabled true` to enable it. See https://git.io/vNItf for more information.

As it stands right now, unless I have internet access, it becomes hard to figure out how to enable it. And if the user is not interested in more info, just to enable the feature, this would make the flow the quickest.

Copy link
Member

Choose a reason for hiding this comment

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

I'd do that as a separate change. This fixes a specific bug.

(Really, I'd like to ship an immutable datastore with IPFS that includes the docs. Then, we could just tell people to read, e.g., /ipfs/QmId/...).

Copy link
Member

Choose a reason for hiding this comment

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

@Stebalien even with that, it's way simpler just to tell what has to be done by the user to achieve what they initially wanted to do, rather than having the indirection of going to a webpage/cat a specific file.

Copy link
Contributor

Choose a reason for hiding this comment

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

@victorbjelkholm for this particular feature there isn't anything useful written in the docs, but in general I think it is better to point people to docs when enabling an experimental feature so they know the risks (if any) invoiced rather then blindly following some instructions in an error message.

@whyrusleeping whyrusleeping merged commit 10b96a3 into ipfs:master Feb 14, 2018
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