Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

[WIP] feat: read config from repo #1372

Closed
wants to merge 2 commits into from
Closed

Conversation

achingbrain
Copy link
Member

Lets us do things like jsipfs config --bool EXPERIMENTAL.pubsub true and have IPFS
respect the flags in daemon and non-daemon mode

Attempts to address #738

@ghost ghost assigned achingbrain May 30, 2018
@ghost ghost added the status/in-progress In progress label May 30, 2018
waterfall([
(done) => this._repo.config.get((error, config) => {
if (error) {
this.log('Could not load config', error)
Copy link
Contributor

Choose a reason for hiding this comment

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

It’s probably not a huge deal since the logs only show if someone’s explicitly set DEBUG=jsipfs, but this will log even when things are just fine — e.g. if running jsipfs init. It makes it look like something went wrong even when the error is expected and OK.

Would it be better to check whether the repo exists first or to check this._options.init? (Are there other situations when this should fail?)

@achingbrain achingbrain force-pushed the read-config-from-repo branch 5 times, most recently from 60525ef to d1751ea Compare July 20, 2018 15:17
@ghost ghost assigned victorb Aug 28, 2018
@victorb
Copy link
Member

victorb commented Aug 28, 2018

@achingbrain resolved the merge conflict here so we could have a CI run, hope you don't mind (change).

@victorb
Copy link
Member

victorb commented Aug 28, 2018

Doesn't actually solve the issue though, where running node src/cli/bin.js config --bool EXPERIMENTAL.pubsub true correctly sets it in the repo config but doesn't actually pass it to the daemon when starting via CLI. Same goes for the preload.enabled config, which is what I first saw this issue with.

Think this PR would feel good by having a test case added to it as well.

is: 1,
then: Joi.boolean().valid(false).required(),
otherwise: Joi.boolean().valid(false)
}),
Copy link
Member

Choose a reason for hiding this comment

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

Eek, bad merge? This shouldn't be here anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for catching that, resolved.

@victorb
Copy link
Member

victorb commented Aug 29, 2018

Seems to work, at least when setting the EXPERIMENTAL.pubsub config value to true, added a test case for that as well that is passing.

Quest goes on for what happens with preload.enabled

Edit: opened #1529 for the preload config issue.

achingbrain and others added 2 commits October 5, 2018 08:24
Lets us do things like `jsipfs config --bool EXPERIMENTAL.pubsub true` and have IPFS
respect the flags in daemon and non-daemon mode
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

It looks to me as though the best place for reading the config from the repo would be in boot.js directly after the repo has been opened and initialized.

In the options passed to the IPFS constructor, the config property is meant to contain serializable config that you'd usually find in ~/.jsipfs/config.

So, I think that rather than extending options with the contents of ~/.jsipfs/config we should be extending options.config.

The special case is the "EXPERIMENTAL" config option which is also a top level property of the options object passed to the constructor, so we need to ensure it is merged with the repo config and that the merged result set on options.EXPERIMENTAL as well as options.config.EXPERIMENTAL 🙄

Just FYI, in pre-start.js any additional config passed in options.config is saved to the config file. Do you think this should be moved to boot.js as well? It seems like an odd task to perform immediately before the node starts...

If we do this we'll be able to clean up the double options/config checking in libp2p which will be really nice.

@daviddias daviddias removed their request for review October 27, 2018 19:56
@daviddias daviddias changed the title feat: read config from repo [WIP] feat: read config from repo Oct 31, 2018
@achingbrain achingbrain closed this Jul 2, 2020
@achingbrain achingbrain deleted the read-config-from-repo branch July 2, 2020 14:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants