Add additional publish function for more fine grained control of IPNS record publishing #91

bonedaddy · 2018-06-23T02:01:00Z

Hello,

I recently ran into an issue with go-ipfs-api where I wanted additional control over IPNS record publishing (key specification, ttl, etc..., retrieving the response) so I have created an additional function PublishWithDetails, which returns the response from the IPFS daemon as well as allows for manipulation of the api parameters.

magik6k

Thanks!

I'm not exactly in favour of adding new stuff here as it will eventually get replaced with coreapi implementation, but this seems useful enough.

Few things:

Some stuff in code
A test or two would be great
- currently we don't test ipns here, and it might be a bit flaky, so feel free to skip this part.
Instead of erroring on missing args, I'd make the arguments optional

magik6k · 2018-06-26T10:49:33Z

ipns.go

+		resolveString = "false"
+	}
+	args := []string{contentHash, resolveString, lifetime, ttl, key}
+	resp, err := s.newRequest(context.TODO(), "name/publish", args...).Send(s.httpcli)


should be context.Background() as in other places

magik6k · 2018-06-26T10:53:18Z

ipns.go

+	if contentHash == "" {
+		return nil, errors.New("empty contentHash provided")
+	}
+	if lifetime == "" {


I'd pass this as time.Duration

magik6k · 2018-06-26T10:53:33Z

ipns.go

+	if lifetime == "" {
+		return nil, errors.New("empty lifetime provided")
+	}
+	if ttl == "" {


Same with time.Duration

magik6k · 2018-06-26T10:54:20Z

ipns.go

+	if key == "" {
+		return nil, errors.New("empty key provided")
+	}
+	if resolve {


can use strconv.FormatBool

bonedaddy · 2018-06-26T22:30:11Z

Thanks for the review, all good suggestions. I'll implement the changes you suggested and try to spec out some tests for this function (I don't think this part should be toooo difficult since it's just one function call). Depending on how busy my work week is I should have this done ~Friday. Thanks again.

At the expense of slight duplicated code, I create two new functions getURLIPNS and SendIPNS to accommodate for how the daemon handles web requests. Currently when using `Send` every argument gets formatted with `arg=....&arg=....` causing defaults to be used for many arguments. This was preventing publishing of records using non-self keys as the arguments for the API calls have to be named.

bonedaddy · 2018-06-27T02:25:50Z

@magik6k So I had to create some slight duplicated code. While writing the tests, I discovered that no matter what, I was unable to sign IPNS entries with keys other than self. I played around with the code and it turned that that when calling Send on the the Request type, the getURL function formats all the provided arguments like so arg=.....&arg=....

so when making the API call you had a URL that looked like

http://localhost:5001/api/v0/name/publish?arg=QmZTR5bcpQD7cFgTorqxZDYaew1Wqgfbd2ud9QqGPAkK2V&arg=true&arg=1h&arg=1h&argkey

instead of

http://localhost:5001/api/v0/name/publish?arg=QmZTR5bcpQD7cFgTorqxZDYaew1Wqgfbd2ud9QqGPAkK2V&resolve=true&lifetime=1h&ttl=1h&key=key

This would cause system defaults to be used for the key, lifetime, ttl, etc....

I was able to get around this without impacting any existing functionality by creating special functions to be called when publishing IPNS entries. After making the changes I was able to publish IPNS entries using any keys.

magik6k · 2018-06-27T12:42:39Z

Did you try to use req.Opts, like in there:

go-ipfs-api/shell.go

Lines 172 to 186 in de157ca

    
           req := NewRequest(context.Background(), s.url, "add") 
        
           req.Body = fileReader 
        
           req.Opts["progress"] = "false" 
        
           if !pin { 
        
           	req.Opts["pin"] = "false" 
        
           } 
        
           if rawLeaves { 
        
           	req.Opts["raw-leaves"] = "true" 
        
           } 
        
           resp, err := req.Send(s.httpcli) 
        
           if err != nil { 
        
           	return "", err 
        
           }

bonedaddy · 2018-06-27T16:17:20Z

Oh I'm dumb haha I didn't see that! I'll try with that, thanks.

Update: Works perfectly! Thanks for the suggestion much cleaner now.

magik6k

Thanks for the effort!

I'd just skip the tests (with t.Skip() at the begging of each test), as I can't think of an easy way of executing them without interfering with the daemon.

Other than that, one nitpick and it should be good to go!

magik6k · 2018-06-27T17:18:55Z

ipns_test.go

+var examplesHash = "/ipfs/Qmbu7x6gJbsKDcseQv66pSbUcAA3Au6f7MfTYVXwvBxN2K"
+
+func TestPublishDetailsWithKey(t *testing.T) {
+	shell := ipfsapi.NewShell("localhost:5001")


other tests do NewShell(shellUrl)

(Note that you'll need to change the package to just shell)

magik6k · 2018-06-27T17:21:02Z

ipns_test.go

+func TestPublishDetailsWithKey(t *testing.T) {
+	shell := ipfsapi.NewShell("localhost:5001")
+
+	resp, err := shell.PublishWithDetails(examplesHash, "key1", time.Hour, time.Hour, false)


This expects key1 to be generated when calling this

magik6k · 2018-06-27T17:22:34Z

ipns_test.go

+func TestPublishDetailsWithoutKey(t *testing.T) {
+	shell := ipfsapi.NewShell("localhost:5001")
+
+	resp, err := shell.PublishWithDetails(examplesHash, "", time.Hour, time.Hour, false)


This will publish over whatever is published on the machine it's invoked on, which is.. suboptimal

magik6k · 2018-06-27T17:27:04Z

ipns.go

+		key = "self"
+	}
+	req.Opts["key"] = key
+	req.Opts["lifetime"] = lifetime.String()


I'd wrap this and ttl with an if and only set this when duration is > 0

go-ipfs cares whether or not a option is present - https://github.com/ipfs/go-ipfs/blob/master/core/commands/publish.go#L117

bonedaddy · 2018-06-27T21:39:04Z

@magik6k No problem! Happy to be of some help, you all have been doing fantastic work with IPFS and the related protocols, kudos. I added t.Skip() to the beginning of the tests so we don't interfere with any daemons.

I haven't done complete tests on this yet and its not really related to this PR so feel free to ignore, but it appears that if my node tries to publish an IPNS entry for content that it does not have locally via cache or pinning then the node hangs on the publishing process indefinitely. I tried this last week and I let my node run for 15 minutes before killing the process.

magik6k

LGTM (with 1 tiny nitpick)

magik6k · 2018-07-03T22:03:41Z

ipns.go

@@ -25,6 +33,36 @@ func (s *Shell) Publish(node string, value string) error {
 	return nil
 }

+// PublishWithDetails is used for fine grained control over record publishing
+func (s *Shell) PublishWithDetails(contentHash, key string, lifetime, ttl time.Duration, resolve bool) (*PublishResponse, error) {
+	var pubResp PublishResponse


I'd put it above/closer to json.Unmarshal line

magik6k · 2018-07-04T20:23:33Z

(needs gofmt, see Travis)

bonedaddy · 2018-07-04T20:59:15Z

Done! Thank you for putting up with some of my silly mistakes! If I submit another PR in the future, I will make sure to thoroughly parse it before-hand to make sure the stuff that came up in this PR, doesn't come up in the future.

magik6k · 2018-07-06T11:06:55Z

Looks good, going to merge now.

Also a note - Commit messages should be scoped by roughly what subsystem they affect, like ipns: ran gofmt instead of just ran gofmt. If you can't think of a good commit name you can usually just merge it with a previous commit (git commit --amend [--no-edit]). Force pushing to PRs is totally fine.

bonedaddy · 2018-07-06T16:51:30Z

Thanks once again! The advice is greatly appreciated 😀

bonedaddy added 2 commits June 22, 2018 18:54

added additional publish method

a9ebe95

removed print statements

1c2767e

Stebalien requested review from a user and magik6k June 26, 2018 03:37

magik6k reviewed Jun 26, 2018

View reviewed changes

bonedaddy mentioned this pull request Jun 27, 2018

IPNS record publishing not doing it under the correct key RTradeLtd/Temporal#117

Closed

removed duplicated code, made use of req.Opts

18615a8

magik6k reviewed Jun 27, 2018

View reviewed changes

updated tests, slight changes to syntax.

980c8a5

magik6k approved these changes Jul 3, 2018

View reviewed changes

added small change

a67bcdb

soupdiver mentioned this pull request Jul 4, 2018

Any way to provide the key to use when publishing to IPNS? #95

Closed

magik6k requested a review from Stebalien July 4, 2018 12:11

ran gofmt

739f959

magik6k merged commit 9984da6 into ipfs:master Jul 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional publish function for more fine grained control of IPNS record publishing #91

Add additional publish function for more fine grained control of IPNS record publishing #91

bonedaddy commented Jun 23, 2018

magik6k left a comment •

edited

Loading

magik6k Jun 26, 2018

magik6k Jun 26, 2018

magik6k Jun 26, 2018

magik6k Jun 26, 2018

bonedaddy commented Jun 26, 2018

bonedaddy commented Jun 27, 2018 •

edited

Loading

magik6k commented Jun 27, 2018

bonedaddy commented Jun 27, 2018 •

edited

Loading

magik6k left a comment

magik6k Jun 27, 2018

magik6k Jun 27, 2018

magik6k Jun 27, 2018

magik6k Jun 27, 2018

magik6k Jun 27, 2018

bonedaddy commented Jun 27, 2018 •

edited

Loading

magik6k left a comment

magik6k Jul 3, 2018

bonedaddy Jul 3, 2018

magik6k commented Jul 4, 2018

bonedaddy commented Jul 4, 2018

magik6k commented Jul 6, 2018

bonedaddy commented Jul 6, 2018

Add additional publish function for more fine grained control of IPNS record publishing #91

Add additional publish function for more fine grained control of IPNS record publishing #91

Conversation

bonedaddy commented Jun 23, 2018

magik6k left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonedaddy commented Jun 26, 2018

bonedaddy commented Jun 27, 2018 • edited Loading

magik6k commented Jun 27, 2018

bonedaddy commented Jun 27, 2018 • edited Loading

magik6k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonedaddy commented Jun 27, 2018 • edited Loading

magik6k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magik6k commented Jul 4, 2018

bonedaddy commented Jul 4, 2018

magik6k commented Jul 6, 2018

bonedaddy commented Jul 6, 2018

magik6k left a comment •

edited

Loading

bonedaddy commented Jun 27, 2018 •

edited

Loading

bonedaddy commented Jun 27, 2018 •

edited

Loading

bonedaddy commented Jun 27, 2018 •

edited

Loading