Initial config spec #93

mvid · 2019-03-29T23:25:07Z

Created to address #63

vyzo · 2019-03-30T07:09:44Z

specs/CONFIG.md

+        * Array[Maddr String]
+        * `[]`
+* `DHT`
+    * `Enabled`


I would rather call this Server and Client instead of ClientMode and Enabled. ClientMode also enabls the dht.

Unless there is an implication that we need to set both Enabled and ClientMode to get the dht client behaviour.

At least in the Go implementation, the don't seem to depend on each other. Preferably it would be one value that would be chosen from an Enum, though, unless it is possible to have both enabled at the same time?

vyzo · 2019-03-30T07:11:01Z

specs/CONFIG.md

+{
+  "ListenAddr": "/unix/tmp/p2pd.sock",
+  "Quiet": false,
+  "ID": "",


this should probably be missing altogether instead of being empty.

In terms of real JSON, that is true. But for this "default config" example, do you think we should strike empty labels?

maybe rather than having this be a "default" example have it be a "complete" example with all of the fields filled in. as a developer, i find those a lot more helpful, because it lets me know what options are possible at a glance! obv the schema does this, but i think that's useful. we could include the "default" config below it, with empty options elided as suggested.

vyzo · 2019-03-30T07:11:31Z

specs/CONFIG.md

+  "ID": "",
+  "Bootstrap": {
+    "Enabled": false,
+    "Peers": []


semantically this looks odd when enabled (the empty Peers list).

It should probably be elided.

mvid · 2019-03-30T20:32:47Z

@jacobheun @vasco-santos @vyzo @bigs + anyone I missed

Any thoughts on how this config should be maintained, and if it should be generated? I was thinking of generating the conf as a protobuf message, since that could be shared between language implementations.

vyzo · 2019-03-31T08:46:19Z

Why not just json? We don't gain anything by using protobuf.

bigs · 2019-03-31T16:11:35Z

our thinking was to make it seamless for all language implementations to have the same serialization

…

On Sun, Mar 31, 2019 at 04:46 vyzo ***@***.***> wrote: Why not just json? We don't gain anything by using protobuf. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#93 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANBWupL2zfarkU0MC2XjJDWfQ6K3m0tks5vcHXcgaJpZM4cTZPR> .

vyzo · 2019-03-31T18:24:18Z

json already does that.

mvid · 2019-03-31T20:09:12Z

JSON doesn't generate the structs we will use to hold the config in Go. There are some third party libs we could use to generate Go structs from JSON, but at that point why would we not use proto like we do everywhere else? If we don't want to generate, then anytime the JSON spec changes, we will also need to change all of the individual language implementations for daemons too.

If we don't care about that, then I will get started on implementation that accepts standard JSON and marshals it into a Go defined struct. I thought I the discussion was worth having.

vyzo · 2019-04-01T07:42:46Z

I don't understand this. We will have to write the structs by hand anyway or write the protobuf for it.
I am very much against using protobuf for configuration -- configuration should always be textual.

jacobheun · 2019-04-01T08:05:34Z

I agree that we should just use json. Having textual configuration makes it a lot easier for users to be able to view/edit/create configurations with a simple editor. Needing to leverage a generator or processor would feel irritating to me as a user trying to set up a new daemon, or to even just view the config for a running daemon.

vasco-santos · 2019-04-01T09:00:45Z

+1 on JSON config here

bigs · 2019-04-01T13:51:11Z

totally down, but to be clear the idea was to use protobuf’s JSON serializer/deserializer. just use protobuf as source of truth for the structs. i agree it’s a bit overkill.

raulk · 2019-04-03T21:04:34Z

What I’m understanding is that we wanted to use protobuf to define an interoperable schema. I think defining a schema is a great idea. How about we write a JSON Schema file for that, and make it part of the specs?

bigs · 2019-04-03T21:11:09Z

@raulk perfect!

vasco-santos · 2019-04-09T10:11:05Z

specs/CONFIG.md

+        * List of bootstrap peers; defaults to the IPFS DHT peers
+        * Array[Maddr String]
+        * `[]`
+* `DHT`


As discussed earlier, we should be able to have more control on the DHT configuration.

For example, we have the interop tests for the DHT content fetching blocked by not being able to add custom validators or selectors. Moreover, I think we should be able to add other options to the DHT, such as the random walk config libp2p/js-libp2p-kad-dht/src/index.js#L39-L42 and the constants libp2p/js-libp2p-kad-dhtmaster/src/constants.js#L32-L39. The constants being configurable is a topic that was already discussed a few times in the context of experiments with the testbed.

That sounds good, but it also sounds like it's out of scope of this particular ticket. This config should be for the daemon as it currently stands. Any feature additions to the deamon will also need to include appropriate config edits

perhaps it should be its own sub object. its absence represents it being disabled. even if it only has one or two options to start. thoughts?

All right, that sounds correct for me! Just mentioned to not fall out of the radar!

It is also important to consider that as we are creating the config now, I think we should at least prepare the dht options to get the remaining configs. Otherwise, if we will expect a string for now, we will create a breaking change afterward.

I edited the DHT property to be an object, and added a field Mode that is an enum of "client", "full" and empty string. It defaults to empty string. It should be easier to add extra fields now.

vyzo · 2019-04-13T07:58:12Z

Can you rebase? There are two new options for the pprof handler. Not sure how well this will play with the generic (cross-daemon) nature of the config, as this is a pretty go-specific thing.

mvid · 2019-04-16T18:31:45Z

@vyzo rebased and included those new options

bigs

will give a more thorough review in the am but this is looking very good

bigs · 2019-04-09T20:43:50Z

specs/CONFIG.md

+        * List of bootstrap peers; defaults to the IPFS DHT peers
+        * Array[Maddr String]
+        * `[]`
+* `DHT`


perhaps it should be its own sub object. its absence represents it being disabled. even if it only has one or two options to start. thoughts?

bigs · 2019-04-17T04:36:29Z

config/config.go

+	return nil
+}
+
+type bootstrap struct {


feels like these should be public, but i'm open to suggestion!

second that.

Is there a reason as to why? My thought was that they were only being used internally for marshaling

vyzo

Command line options should not be ignored when there is a config, but rather override options.

vyzo · 2019-04-17T07:30:45Z

config/config.go

+		return err
+	}
+	jma := JSONMaddr{ma}
+	jm = &jma


just jm = &JSONMaddr(ma}

vyzo · 2019-04-17T07:31:29Z

config/config.go

+	return nil
+}
+
+type bootstrap struct {


second that.

vyzo · 2019-04-17T07:34:47Z

p2pd/main.go

+		if err := json.Unmarshal(body, c); err != nil {
+			log.Fatal(err)
+		}
+	} else {


we need to allow command line options to override the config.

i think we could make that in another change, so long as we note the behavior in the readme. maybe we just make an issue for that to support config merging. this would be a big help in the meantime

I think we should do it here, it's a pretty simple thing to do. It would be very surprising to pass options that are ignored when also passing a config.

sounds good let’s do overrides

bigs

great work, thanks for taking this on. just a few changes requested

bigs · 2019-04-18T19:56:02Z

p2pd/main.go

+		if err != nil {
+			log.Fatal(err)
+		}
+		if err := json.Unmarshal(body, c); err != nil {


i think this (and the instance below) should be &c

bigs · 2019-04-18T19:56:41Z

p2pd/main.go

+		if err := json.Unmarshal(body, c); err != nil {
+			log.Fatal(err)
+		}
+	} else {


i think we could make that in another change, so long as we note the behavior in the readme. maybe we just make an issue for that to support config merging. this would be a big help in the meantime

bigs · 2019-04-18T19:59:46Z

specs/CONFIG.md

+{
+  "ListenAddr": "/unix/tmp/p2pd.sock",
+  "Quiet": false,
+  "ID": "",


maybe rather than having this be a "default" example have it be a "complete" example with all of the fields filled in. as a developer, i find those a lot more helpful, because it lets me know what options are possible at a glance! obv the schema does this, but i think that's useful. we could include the "default" config below it, with empty options elided as suggested.

mvid · 2019-04-18T20:15:32Z

Is it possible to make flags order dependent in Go? Perhaps we give preference to whatever is provided last

…

On Apr 18, 2019, at 22:05, vyzo ***@***.***> wrote: @vyzo commented on this pull request. In p2pd/main.go: > if err != nil { log.Fatal(err) } + if err := json.Unmarshal(body, c); err != nil { + log.Fatal(err) + } + } else { I think we should do it here, it's a pretty simple thing to do. It would be very surprising to pass options that are ignored when also passing a config. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

vyzo · 2019-04-18T20:16:38Z

Is it possible to make flags order dependent in Go?

not that I know of.

mvid · 2019-04-19T06:13:48Z

@bigs relay, pubsub, connectionManager and bootstrap all have Enabled fields in their subobjects. In theory they could all be replaced with subobject existence instead. I feel like being explicit is better though. Right now you need to pass the enabled flag for any of the subflags to be used

bigs · 2019-04-22T22:08:28Z

@mvid i like the explicit nature, let's keep it. let's do the command line overrides and merge this

mvid · 2019-04-23T20:47:53Z

@bigs ok, it now overrides with flags

bigs

looks great

mvid requested a review from bigs March 29, 2019 23:25

ghost assigned mvid Mar 29, 2019

ghost added the in progress label Mar 29, 2019

vyzo reviewed Mar 30, 2019

View reviewed changes

mvid force-pushed the spec/config branch from b8004fa to e9860a8 Compare April 3, 2019 02:01

vasco-santos reviewed Apr 9, 2019

View reviewed changes

mvid added 9 commits April 16, 2019 18:52

initial config spec

8ddd8be

modify daemon to use config files

b450d7d

string type not nil

dc47fd2

go fmt

5b3e88d

proper type assertion

c04c979

schema json

ec4de67

enum on dht mode

17981db

gitignore

66c74fb

updates for pprof

6044ec4

mvid force-pushed the spec/config branch from b749c2d to 6044ec4 Compare April 16, 2019 18:28

fmt

f6b9de0

mvid requested review from vasco-santos and vyzo and removed request for vasco-santos April 16, 2019 18:38

bigs reviewed Apr 17, 2019

View reviewed changes

vyzo requested changes Apr 17, 2019

View reviewed changes

bigs suggested changes Apr 18, 2019

View reviewed changes

review comments

3fbe587

override with flags

234661e

export config structs

ad5d34f

bigs approved these changes Apr 23, 2019

View reviewed changes

bigs merged commit 6f4d625 into master Apr 23, 2019

ghost removed the in progress label Apr 23, 2019

bigs deleted the spec/config branch April 23, 2019 21:38

vasco-santos mentioned this pull request May 24, 2019

Add support for using a config file libp2p/js-libp2p-daemon#18

Open

Initial config spec #93

Initial config spec #93

Conversation

mvid commented Mar 29, 2019 • edited Loading

vyzo Mar 30, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvid commented Mar 30, 2019

vyzo commented Mar 31, 2019

bigs commented Mar 31, 2019 via email

vyzo commented Mar 31, 2019

mvid commented Mar 31, 2019

vyzo commented Apr 1, 2019

jacobheun commented Apr 1, 2019

vasco-santos commented Apr 1, 2019

bigs commented Apr 1, 2019

raulk commented Apr 3, 2019

bigs commented Apr 3, 2019

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

vyzo commented Apr 13, 2019

mvid commented Apr 16, 2019

bigs 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

vyzo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigs 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

mvid commented Apr 18, 2019 via email

vyzo commented Apr 18, 2019

mvid commented Apr 19, 2019

bigs commented Apr 22, 2019

mvid commented Apr 23, 2019

bigs left a comment

Choose a reason for hiding this comment

mvid commented Mar 29, 2019 •

edited

Loading

vyzo Mar 30, 2019 •

edited

Loading