feat: allow daemon to init and start in a single cmd #2428

hugomrdias · 2019-09-05T10:19:33Z

This PR aligns ipfs init and ipfs daemon with go-ipfs.

ipfs init changed to accept a file path as an argument

ipfs daemon changed to support --init and --init-config options.

Now we can do ipfs daemon --init --init-config /path/to/custom-config

Missed the breaking change in the commit msg...

BREAKING CHANGE: ipfs init first positional argument is a path to a file instead of a JSON string. Although the previous version was completely broken.

achingbrain · 2019-09-05T10:48:47Z

It would be nicer if it just init'd the repo if it needed to without having to pass flags or anything else.

The new user experience is:

$ ipfs
/// blah blah blah blah long thing don't make me read
$ ipfs daemon
// NO! ERROR!  Run this other command
$ ipfs init
// THANK YOU HUMAN
$ ipfs daemon
...

achingbrain · 2019-09-05T11:03:10Z

What I'm saying is, I think we should just init when required instead of adding more flags like this.

Running ipfs init would then be a no-op, unless maybe we want to let the user re-init, in which case something like ipfs init --force might be appropriate?

alanshaw · 2019-09-05T11:32:25Z

This is really cool and I'm excited to see the speed increase in our CLI tests.

However, I'm struggling to think of a reason why we wouldn't want ipfs daemon to auto-init if not initialised already. Is it a barrier for new users for no reason? @Stebalien @djdv what do you think?

So, how about: remove --init (just auto initialise) but retain --init-config so that we can still pass config?

Stebalien · 2019-09-05T15:25:40Z

However, I'm struggling to think of a reason why we wouldn't want ipfs daemon to auto-init if not initialised already.

I agree it should be initialized by default. However, the one reason not to do this is that the current behavior prevents the daemon from being run against the wrong repo. In my case, I always set an explicit repo with IPFS_PATH. My user's repo contains an api file (for my system-wide daemon) and that's it.

But yeah, there's really no reason to not auto initialize.

hugomrdias · 2019-09-09T10:18:44Z

Should we get this in as is or change it ?

achingbrain · 2019-09-09T10:20:29Z

I think we should change this to initialise by default instead of adding extra flags.

hugomrdias · 2019-09-10T09:21:14Z

done can we get this merged ?

src/cli/commands/init.js

test/cli/daemon.js

Also tag last successful build and ensure we do not get stealth multiaddr@v7

achingbrain · 2019-09-13T18:37:59Z

test/cli/daemon.js

@@ -265,4 +282,34 @@ describe('daemon', () => {
 expect(err.stdout).to.include(`Node.js version: ${process.versions.node}`)
 }
 })
+
+ it('should init by default if not already', async function () {


How does this test assert that the daemon auto-initialised it's own repo?

ipfs daemon cmd throws if there is no repo initialised. So if this test passes we should be good.

It should also assert that the repo was created after the command has run, otherwise it doesn't actually test anything other than that the daemon process was killed. Check the repo directory exists or something like that.

My reasoning is to not over test stuff, here we are testing the daemon command and logic, init was added to that logic but we should only test that it gets ran not that init works correctly we have other tests for that.
This test will fail if init isn't ran or if init fails internally, which is the scope i wanted when i wrote the test.
Makes sense ?

I guess its a bit hidden because init is on by default and if init doesn't work daemon also fails completely but still we have other tests that make sure init creates a repo.

If this test doesn't test anything not covered by other tests it should be removed.

If it does test something not covered by other tests, it should contain assertions to prove the functionality being tested works as expected.

Too many of our tests don't actually test anything, they just ensure that nothing on the happy path explodes so aren't all that useful.

@achingbrain is right. Tests should be both valuable and informative. If it requires one to know that the test is testing for the program not to blow up in some weird situation, at least that needs to be well documented and not just live in one persons brain.

the assertion is this if (stdout.includes('Daemon is ready')) { i agree this isn't the best way to do things and i will improve this in a near future, but this is the way we assert a daemon ran successfully in the others tests.

this is the first test to work without calling ipfs(init).

either i remove the test or change the name to something more informative like 'should start the daemon and init automatically without calling init first' or something else but i don't think testing for the repo is useful here.
im open to suggestions

Suggestions were:

Document it with code comments

Assert that the repo was created as part of the test

This PR aligns `ipfs init` and `ipfs daemon` with go-ipfs. ipfs/kubo#6489 `ipfs init` changed to accept a file path as an argument `ipfs daemon` changed to support `--init` and `--init-config` options. Now we can do `ipfs daemon --init --init-config /path/to/custom-config` refs: ipfs/js-ipfsd-ctl#303

BREAKING CHANGE: swarm tests need promise based setup check PRs below the following PRs add support async setup: ipfs-inactive/js-ipfs-http-client#1105 ipfs/js-ipfs#2428

this can be improve directly in aegir

hugomrdias requested review from alanshaw and achingbrain September 5, 2019 10:20

alanshaw approved these changes Sep 5, 2019

View reviewed changes

hugomrdias force-pushed the feat/daemon-start-init branch from 84a8b34 to 0decf84 Compare September 6, 2019 15:35

hugomrdias mentioned this pull request Sep 6, 2019

feat: init start with one cmd ipfs/js-ipfsd-ctl#369

Closed

daviddias reviewed Sep 10, 2019

View reviewed changes

src/cli/commands/init.js Show resolved Hide resolved

daviddias reviewed Sep 10, 2019

View reviewed changes

test/cli/daemon.js Outdated Show resolved Hide resolved

daviddias mentioned this pull request Sep 10, 2019

⚡️ v0.38.0 RELEASE 🚀 #2396

Closed

66 tasks

hugomrdias force-pushed the feat/daemon-start-init branch from 8471234 to 5e39dc7 Compare September 11, 2019 15:02

chore: run interop tests as part of the build

9b03f2f

Also tag last successful build and ensure we do not get stealth multiaddr@v7

achingbrain suggested changes Sep 13, 2019

View reviewed changes

hugomrdias added 7 commits September 15, 2019 22:38

fix: fix the expected result

4627c62

fix: fix the expected result

026dea5

chore: test windows

bad8bce

fix: fix tests on windows

edcdcca

fix: auto init and remove --init flag

2266446

chore: rename param

1bdff33

hugomrdias mentioned this pull request Sep 16, 2019

feat: add support for async/await setup in the swarm tests ipfs-inactive/interface-js-ipfs-core#529

Merged

hugomrdias added 4 commits September 16, 2019 12:46

feat: add support for async/await setup

8cc564b

fix: fix electron setup

4d20763

fix: upgrade deps to match

9e83e3f

chore: fix linter

d134357

hugomrdias force-pushed the feat/daemon-start-init branch from 5e39dc7 to d134357 Compare September 16, 2019 11:48

chore: upgrade deps

0984567

achingbrain force-pushed the master branch from 9b03f2f to 7cb08c2 Compare September 17, 2019 17:15

hugomrdias added 4 commits September 18, 2019 12:13

fix: remove bits

0a9ba6e

fix: update deps and some test tweaks

91bfe15

chore: fix isNode

bb91c2e

fix: fix interop tests

68ff2ce

this can be improve directly in aegir

achingbrain mentioned this pull request Sep 25, 2019

fix: topicToKey should return a buffer not a string ipfs/js-datastore-pubsub#16

Merged

achingbrain added 2 commits September 25, 2019 16:31

test: add assertions to test to ensure daemon auto-initted

7faf0da

Merge branch 'master' into feat/daemon-start-init

7bd8ef8

achingbrain approved these changes Sep 25, 2019

View reviewed changes

achingbrain merged commit 16d5e7b into master Sep 25, 2019

achingbrain deleted the feat/daemon-start-init branch September 25, 2019 15:35

hugomrdias mentioned this pull request Sep 25, 2019

Improve overall state of js-ipfs tests #2276

Closed

42 tasks

This was referenced Sep 25, 2019

⚡️ v0.39.0 RELEASE 🚀 #2489

Closed

Add config profile endpoint and CLI #2165

Merged

snyk-bot mentioned this pull request Dec 6, 2019

[Snyk] Upgrade ipfs from 0.37.1 to 0.39.0 dyna-dot/orbit-web#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow daemon to init and start in a single cmd #2428

feat: allow daemon to init and start in a single cmd #2428

hugomrdias commented Sep 5, 2019 •

edited

Loading

achingbrain commented Sep 5, 2019

achingbrain commented Sep 5, 2019

alanshaw commented Sep 5, 2019

Stebalien commented Sep 5, 2019

hugomrdias commented Sep 9, 2019

achingbrain commented Sep 9, 2019

hugomrdias commented Sep 10, 2019

achingbrain Sep 13, 2019

hugomrdias Sep 16, 2019

achingbrain Sep 16, 2019

hugomrdias Sep 16, 2019

hugomrdias Sep 16, 2019

achingbrain Sep 16, 2019

daviddias Sep 16, 2019

hugomrdias Sep 16, 2019

daviddias Sep 17, 2019

feat: allow daemon to init and start in a single cmd #2428

feat: allow daemon to init and start in a single cmd #2428

Conversation

hugomrdias commented Sep 5, 2019 • edited Loading

achingbrain commented Sep 5, 2019

achingbrain commented Sep 5, 2019

alanshaw commented Sep 5, 2019

Stebalien commented Sep 5, 2019

hugomrdias commented Sep 9, 2019

achingbrain commented Sep 9, 2019

hugomrdias commented Sep 10, 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

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

hugomrdias commented Sep 5, 2019 •

edited

Loading