CoreAPI: Global offline option by magik6k · Pull Request #5825 · ipfs/kubo

magik6k · 2018-12-06T21:39:29Z

For #5654. This implements the 'Functional options on the constructor' approach.

I decided to not use core.IpfsNode directly in coreapi as this way we have much better control over how the internals are used. Longer term, after coreapi interface is extracted form go-ipfs, I plan on merging a lot of core node and coreapi functionality, but I want to do that together with go-ipfs constructor cleanup as it will be quite breaking for people using core.IpfsNode directly (there still are good reasons to do that)

A few bugs and todos are still left, reviews welcome

schomatis

The code in general LGTM (but I can't really comment on the design decision of extracting all of the IpfsNode attributes here).

core/coreapi/coreapi.go

Stebalien · 2018-12-11T01:18:25Z

core/coreapi/coreapi.go

+
+	// TODO: this can be generalized to all functions when we implement some
+	// api based security mechanism
+	isPublishAllowed func() error


This method isn't really about security. Really, we should allow publishing IPNS records while /ipns is mounted. We don't currently because we haven't implemented that yet.

I know, I just wanted to hint that something like this may be useful (but a full security layer implementing coreapi interface would probably be better)

core/coreapi/coreapi.go

Stebalien · 2018-12-11T01:40:55Z

core/commands/cmdenv/env.go

 	}

-	return ctx.GetAPI()
+	local, _ := req.Options["local"].(bool)


So, this'll apply this option (correctly) to all commands fixing part of #4182. I wonder if we should fix the "confusing" part while we're at it by renaming this option to "offline" (keeping a hidden alias for compatibility).

That would also allow us to share the same option with ipfs daemon --offline which should help to further reduce confusion.

I'm fine with doing that (the hidden alias could throw warnings at people). This will probably need support on the js side too

I didn't realize js replicated that option.

@alanshaw how would you feel about switching from --local to --offline (keeping --local as a backcompat alias)?

Implemented, waiting for @alanshaw's opinion (assuming js-ipfs implements --local)

core/commands/root.go

core/coreapi/coreapi.go

Stebalien · 2018-12-19T00:25:29Z

core/coreapi/coreapi.go

+
+		cs := cfg.Ipns.ResolveCacheSize
+		if cs == 0 {
+			cs = 128


We should lift this to a constant.

Stebalien · 2018-12-19T00:29:48Z

core/coreapi/unixfs.go

 	dserv := dag.NewDAGService(bserv)

-	fileAdder, err := coreunix.NewAdder(ctx, n.Pinning, n.Blockstore, dserv)
+	fileAdder, err := coreunix.NewAdder(ctx, pinning, addblockstore, dserv)


Is this a bugfix or a bug? It's now using addblocksore where it used to use the unmodified blockstore.

bugfix - coreunix.NewAdder uses it only for the GCLock, which we don't want/need to take from the 'real' node when we do add with OnlyHash set.

Stebalien · 2018-12-19T00:30:42Z

Mostly nits at this point but you should take a look at #5825 (comment).

Stebalien · 2018-12-19T05:04:00Z

@ipfs/go-team this is an interface change (but not a breaking one).

It replaces --local with --offline, leaving --local for backwards compatibility. It also makes --offline work with every command (--local currently only works with a few commands).

Suggestions? Objections?

magik6k · 2018-12-20T14:40:33Z

(rebased)

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

CoreAPI: Global offline option This commit was moved from ipfs/kubo@28dabc7

magik6k requested a review from Kubuxu as a code owner December 6, 2018 21:39

ghost assigned magik6k Dec 6, 2018

ghost added the status/in-progress In progress label Dec 6, 2018

magik6k added the topic/core-api Topic core-api label Dec 6, 2018

magik6k mentioned this pull request Dec 6, 2018

CoreAPI progress tracking issue #4498

Open

51 tasks

Stebalien requested a review from schomatis December 7, 2018 23:03

schomatis reviewed Dec 10, 2018

View reviewed changes

Stebalien reviewed Dec 11, 2018

View reviewed changes

magik6k force-pushed the feat/coreapi/opts branch 2 times, most recently from a16a5ba to 7ddb45f Compare December 12, 2018 12:06

Stebalien mentioned this pull request Dec 13, 2018

Rename --local to --offline ipfs/js-ipfs#1778

Closed

2 tasks

Stebalien reviewed Dec 13, 2018

View reviewed changes

core/commands/root.go Outdated Show resolved Hide resolved

core/coreapi/coreapi.go Outdated Show resolved Hide resolved

magik6k force-pushed the feat/coreapi/opts branch 2 times, most recently from 4eccfa9 to 6de9b03 Compare December 17, 2018 08:15

Stebalien reviewed Dec 18, 2018

View reviewed changes

core/coreapi/coreapi.go Outdated Show resolved Hide resolved

Stebalien reviewed Dec 18, 2018

View reviewed changes

core/coreapi/coreapi.go Outdated Show resolved Hide resolved

Stebalien reviewed Dec 19, 2018

View reviewed changes

Stebalien approved these changes Dec 19, 2018

View reviewed changes

Stebalien added the RFM label Dec 19, 2018

magik6k force-pushed the feat/coreapi/opts branch from 00dddb3 to 81fa251 Compare December 20, 2018 14:24

magik6k added 6 commits December 20, 2018 16:40

coreapi: Global options for api constructor

6e90367

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: Untangle from core.IpfsNode

3183b1c

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: global offline option

c832a32

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi.WithOptions

50aea07

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: drop nameopt.Local in favour of api.Offline

6e18a6b

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: implement --local with Offline option

2c2f9f2

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k added 5 commits December 20, 2018 16:40

coreapi WithOptions: apply on top of parent options

47a4639

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

commands: deprecate --local for --offline

2dceb09

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

commands: don't use -O as global offline

a39c299

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: update for always loaded privkey

190728f

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi offline: Address review

6044d2a

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k force-pushed the feat/coreapi/opts branch from 81fa251 to 6044d2a Compare December 20, 2018 15:42

coreapi: move DefaultIpnsCacheSize to a const

3f0945a

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Stebalien merged commit 28dabc7 into master Dec 20, 2018

ghost removed the status/in-progress In progress label Dec 20, 2018

Stebalien deleted the feat/coreapi/opts branch December 20, 2018 16:24

hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023

Merge pull request ipfs/kubo#5825 from ipfs/feat/coreapi/opts

d6ccc81

CoreAPI: Global offline option This commit was moved from ipfs/kubo@28dabc7

Uh oh!

Conversation

magik6k commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schomatis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stebalien commented Dec 19, 2018

Uh oh!

Stebalien commented Dec 19, 2018

Uh oh!

magik6k commented Dec 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

magik6k commented Dec 6, 2018 •

edited

Loading