Skip to content
This repository has been archived by the owner on Apr 29, 2020. It is now read-only.

Added Group config #29

Merged
merged 10 commits into from
Mar 1, 2016
Merged

Added Group config #29

merged 10 commits into from
Mar 1, 2016

Conversation

RichardLitt
Copy link
Contributor

Some issues:

  • I was unable to get the second argument working, so it is currently not documented how to patch a config entry. I also was unable to meaningfully get bool and json flags working. Wireshark says that encoding=json is added to each command normally run by the CLI, as well as stream-channels=true, but neither of these are mentioned in the CLI mans, which is odd. I'm not sure what they are doing.
  • 'ipfs config edit' opens up a second terminal window - over the running daemon - when requested using curl on the commandline, so I'm not actually sure what that should be doing in the API. I had it originally set as content-type buffer in the request, too, but I'm not sure where I got that; probably Postman. It wasn't shown in Wireshark.
  • Some of the commands returned no body in the CLI when curled, but returned malformed JSON in Wireshark. Not sure what to make of that.

@RichardLitt
Copy link
Contributor Author

I got the arguments working by putting the HTML request into double quotes. D'oh!

@RichardLitt
Copy link
Contributor Author

Ok, now I am simply unable to get bool working. Strangely enough, this works:

curl -i "http://localhost:5001/api/v0/config?arg=Datastore.Path&arg=\"false\"&json"

Also weirdly, so does this:

curl -i "http://localhost:5001/api/v0/config/Datastore.Path"

I've added that second one to it's own section.

@daviddias daviddias mentioned this pull request Feb 2, 2016
14 tasks
@daviddias
Copy link
Contributor

'ipfs config edit' opens up a second terminal window - over the running daemon - when requested using curl on the commandline, so I'm not actually sure what that should be doing in the API

Yeah, that is kind if a cli thing around the ipfs config show and replace (or at least I would expect for it to be that way :) )

Some of the commands returned no body in the CLI when curled, but returned malformed JSON in Wireshark. Not sure what to make of that.

This is probably an API error and the CLI is smart enough to compensate. Consider opening it as a bug in go-ipfs.

## show
+ Parameters
+ arg1: "Datastore.Path" (string, required) - The key of the config entry
+ arg2: "~/.ipfs/datastore" (string, required) - The value to set the config entry to
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a note here saying that it can be any key of the config entry and that Datastore.Path is just an example of one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. That's apiary format for an example; it is clear by the syntax that this is not the only possible option.

@RichardLitt
Copy link
Contributor Author

@diasdavid Updated to fix all of the POST and GET stuff. Also some small syntax fixes.

@RichardLitt
Copy link
Contributor Author

Needs to be checked before #20 can be closed.

+ Parameters
+ arg1: "Datastore.Path" (string, required) - The key of the config entry.
+ arg2: "~/.ipfs/datastore" (string, required) - The value to set the config entry to.
+ bool (boolean, optional) - Set a boolean.
Copy link
Collaborator

Choose a reason for hiding this comment

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

default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@daviddias
Copy link
Contributor

Wireshark says that encoding=json is added to each command normally run by the CLI, as well as stream-channels=true, but neither of these are mentioned in the CLI mans, which is odd. I'm not sure what they are doing.

I think these are just default. //cc @whyrusleeping

@RichardLitt Other than the last comment above, it seems this PR was reviewed twice, could you post a item list with things you would still like feedback?

## show
+ Parameters
+ arg1: "Datastore.Path" (string, required) - The key of the config entry.
+ arg2: "~/.ipfs/datastore" (string, required) - The value to set the config entry to.

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Good catch.

@RichardLitt
Copy link
Contributor Author

I need review or to do:

  • Merge Added defaults to config opts descriptions ipfs/kubo#2412, add defaults.
  • Make it clear whether replace needs to be a path or the file itself. I think the latter, but I am not sure. @whyrusleeping?
  • Should ipfs config edit be included at all? Or should we remove it? What happens if I call in my browser?
  • Open issue about API-wide issues: Method variability, encoding-json and stream-channels=true, malformed JSON responses

@RichardLitt
Copy link
Contributor Author

File and path are synonymous,

@RichardLitt
Copy link
Contributor Author

@diasdavid Should ipfs config edit be included at all? Or should we remove it? What happens if I call in my browser?

@daviddias
Copy link
Contributor

that encoding=json is added to each command normally run by the CLI, as well as stream-channels=true, but neither of these are mentioned in the CLI mans, which is odd. I'm not sure what they are doing.

They are just default for every command, probably at some point in time there was other options, now it is always the same.

'ipfs config edit' opens up a second terminal window - over the running daemon - when requested using curl on the commandline, so I'm not actually sure what that should be doing in the API. I had it originally set as content-type buffer in the request, too, but I'm not sure where I got that; probably Postman. It wasn't shown in Wireshark.

ipfs config edit is almost a convenience on top of show+replace, it is a cli only thing. The only reason why I see it that way is because the way the http-api is done in go-ipfs, it globs any command that is available on core/commands, even if it is not supposed to be an http-api thing.

Should ipfs config edit be included at all? Or should we remove it? What happens if I call in my browser?

I vote to remove it

Some of the commands returned no body in the CLI when curled, but returned malformed JSON in Wireshark. Not sure what to make of that.

Better go one by one and see each case.

@RichardLitt
Copy link
Contributor Author

I removed edit. Ignoring the global flags, for now. Same for the Wireshark responses; I think that's probably me misreading wireshark.

RichardLitt added a commit that referenced this pull request Mar 1, 2016
@RichardLitt RichardLitt merged commit 336f9ff into master Mar 1, 2016
@RichardLitt RichardLitt deleted the feature/config branch March 1, 2016 21:59
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.

5 participants