Refactor IPFS cmds #951

kernelwhisperer · 2019-11-23T15:24:57Z

🦅 Pull Request

2nd iteration of #912 where I fix the --detached flag.

Next would be to add tests and check how others commands use the IPFS logic.

🚨 Test instructions

✔️ PR Todo

Include links to related issues/PRs
Update unit tests for this change
Update the relevant documentation
Clear dependencies on other modules that have to be released before merging this

Imported issues from #952

ipfs data ✔️

There seems to be a problem with 'ipfs-http-client' code when running the test in node versions 8, and 9.

 Could not connect to the IPFS API at "https://ipfs.infura.io:5001"

 connectOrThrow (src/lib/ipfs/misc.js:33:11)
 getClient (src/lib/ipfs/misc.js:11:10)
 _ava.default.beforeEach (test/integration/ipfs/data.test.js:13:23)

When inspecting the real error that throws:

 /home/lion/Code/aragon/aragon-cli/packages/aragon-cli/node_modules/ipfs-http-client/src/files-regular/index.js:60
      for await (const entry of get(path, options)) {
          ^^^^^
 SyntaxError: Unexpected reserved word
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:616:28)
    at Module._compile (/home/lion/Code/aragon/aragon-cli/packages/aragon-cli/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (module.js:663:10)
    at extensions.(anonymous function) (/home/lion/Code/aragon/aragon-cli/packages/aragon-cli/node_modules/require-precompiled/index.js:16:3)
    at newLoader (/home/lion/Code/aragon/aragon-cli/packages/aragon-cli/node_modules/pirates/lib/index.js:104:7)
    at Object.require.extensions.(anonymous function) [as .js] (/home/lion/Code/aragon/aragon-cli/packages/aragon-cli/node_modules/ava/lib/worker/dependency-tracker.js:42:4)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)

Apparently the feature used and referenced above for-await...of is not compatible with node < 10.
Does babel also transpile the node_module dependencies? Does ava transpile the code before testing?

Reported here: ipfs-inactive/js-ipfs-http-client#1190

ipfs daemon ✔️ :

There are two methods to start IPFS: startDaemon and startIPFSDaemon (which is not used by anyone). Is that an error?
Also isIPFSRunning and isDaemonRunning fulfill the same task but only isDaemonRunning is used.
8dc2c36#diff-831fb4c7e020ac556a769ecf3742aff5R28

codecov · 2019-11-23T15:29:50Z

Codecov Report

Merging #951 into develop will increase coverage by 4.44%.
The diff coverage is 51.76%.

@@             Coverage Diff             @@
##           develop     #951      +/-   ##
===========================================
+ Coverage    22.62%   27.07%   +4.44%     
===========================================
  Files          112      119       +7     
  Lines         2453     2471      +18     
===========================================
+ Hits           555      669     +114     
+ Misses        1898     1802      -96

Impacted Files	Coverage Δ
...commands/dao_cmds/acl_cmds/utils/aclExecHandler.js	`0% <ø> (ø)`	⬆️
packages/aragon-cli/src/commands/ipfs_cmds/stop.js	`0% <ø> (ø)`	⬆️
packages/aragon-cli/src/commands/run.js	`0% <ø> (ø)`	⬆️
packages/aragon-cli/src/commands/dao_cmds/exec.js	`0% <ø> (ø)`	⬆️
...ckages/aragon-cli/src/commands/dao_cmds/install.js	`0% <ø> (ø)`	⬆️
packages/aragon-cli/src/cli.js	`0% <ø> (ø)`	⬆️
packages/aragon-cli/src/lib/ipfs/misc.js	`93.33% <ø> (+53.33%)`	⬆️
...ckages/aragon-cli/src/commands/dao_cmds/upgrade.js	`0% <ø> (ø)`	⬆️
packages/aragon-cli/src/commands/dao_cmds/act.js	`0% <ø> (ø)`	⬆️
packages/aragon-cli/src/commands/dao_cmds/new.js	`0% <ø> (ø)`	⬆️
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e08b55...5f734c7. Read the comment docs.

dapplion · 2019-11-24T19:10:17Z

Next would be to add tests and check how others commands use the IPFS logic.

Got you, #952

packages/aragon-cli/src/commands/ipfs_cmds/start.js

dapplion · 2019-11-29T10:45:29Z

packages/aragon-cli/src/commands/ipfs_cmds/start.js

  } = argv

-  await runStartTask({
+  const { processController } = await runStartTask({


I would declare the tasks inside the handler directly. It would make the module shorter, and you won't need to pass a long list of arguments around.

Hmm, good point!

Gonna take a look at how we use listr and take this into consideration too. (In another PR)

dapplion · 2019-11-29T11:03:51Z

@0x6431346e Clean neat PR good job! I've opened a PR against yours with a cosmetic improvement to avoid calling a logger multiple times and use template literals instead #973.

If you believe the change is not relevant for now, please ignore it without hesitation.

kernelwhisperer · 2019-12-01T16:39:34Z

@dapplion @macor161 what do you guys think of this test setup? https://github.com/aragon/aragon-cli/blob/refactor/ipfs-cmds-2nd-try/packages/aragon-cli/scripts/setup-integration-tests.js

We are using the CLI to test the CLI 😄

I think we can go a step further and run these processes on non-standard ports, to avoid conflicts.

dapplion · 2019-12-01T19:22:26Z

@dapplion @macor161 what do you guys think of this test setup? https://github.com/aragon/aragon-cli/blob/refactor/ipfs-cmds-2nd-try/packages/aragon-cli/scripts/setup-integration-tests.js

We are using the CLI to test the CLI

I think we can go a step further and run these processes on non-standard ports, to avoid conflicts.

Yeah looks good! I don't see a problem with using the standard ports tho.

macor161 · 2019-12-02T00:11:33Z

@dapplion @macor161 what do you guys think of this test setup? https://github.com/aragon/aragon-cli/blob/refactor/ipfs-cmds-2nd-try/packages/aragon-cli/scripts/setup-integration-tests.js

I like that, it's going to be very useful for integration tests. For the problems with await/async, perhaps we can consider dropping support for node 8 and 9. Official support will end in December 2019: https://nodejs.org/en/about/releases/

There are a few problems with test snapshots on my laptop. I will have another look tomorrow.

dapplion · 2019-12-02T08:45:44Z

@dapplion @macor161 what do you guys think of this test setup? https://github.com/aragon/aragon-cli/blob/refactor/ipfs-cmds-2nd-try/packages/aragon-cli/scripts/setup-integration-tests.js

I like that, it's going to be very useful for integration tests. For the problems with await/async, perhaps we can consider dropping support for node 8 and 9. Official support will end in December 2019: https://nodejs.org/en/about/releases/

There are a few problems with test snapshots on my laptop. I will have another look tomorrow.

I would be in favor of dropping support for node <10, but we should show a warning message after detecting the user's node version

kernelwhisperer · 2019-12-02T10:52:58Z

I like that, it's going to be very useful for integration tests. For the problems with await/async, perhaps we can consider dropping support for node 8 and 9. Official support will end in December 2019: nodejs.org/en/about/releases

I would be in favor of dropping support for node <10, but we should show a warning message after detecting the user's node version

Makes sense, since support is ending soon and ipfs does not support node < 10, let's drop it.

kernelwhisperer · 2019-12-02T11:37:25Z

Example output:

daniel@zen5-ub:~/r/ar/aragon-cli/packages/aragon-cli$ node dist/index.js
Detected node version: 11.15.0. Required node version: >=12.0.0.

dapplion · 2019-12-02T20:54:43Z

Example output:

daniel@zen5-ub:~/r/ar/aragon-cli/packages/aragon-cli$ node dist/index.js
Detected node version: 11.15.0. Required node version: >=12.0.0.

Yes but we must support versions >= 10

kernelwhisperer · 2019-12-03T12:13:52Z

Example output:

daniel@zen5-ub:~/r/ar/aragon-cli/packages/aragon-cli$ node dist/index.js
Detected node version: 11.15.0. Required node version: >=12.0.0.

Yes but we must support versions >= 10

Just an example, the engine field has the correct value.

0xGabi

Amazing PR. Leave some points to address. Will also double check why I was not able to use commands even all test are passing

packages/aragon-cli/src/lib/node/index.js

packages/aragon-cli/src/lib/node/process.js

packages/aragon-cli/src/index.js

packages/aragon-cli/scripts/setup-integration-tests.js

packages/aragon-cli/src/commands/ipfs_cmds/propagate.js

packages/aragon-cli/package.json

packages/aragon-cli/src/lib/ipfs/config.js

packages/aragon-cli/src/lib/ipfs/misc.js

packages/aragon-cli/test/integration/ipfs/data.test.js

0xGabi · 2019-12-04T19:22:41Z

@0x6431346e I think you are already using ES6 syntax a lot, anyways would you double check with the same process as @dapplion used here: #950

0xGabi

Amazing PR 🙌

kernelwhisperer requested a review from 0xGabi as a code owner November 23, 2019 15:24

0xGabi added the SDK v7 label Nov 24, 2019

kernelwhisperer force-pushed the refactor/ipfs-cmds-2nd-try branch from 6530be3 to 98bb03a Compare November 28, 2019 14:11

0xGabi mentioned this pull request Nov 28, 2019

Refactor codebase for robustness #947

Closed

43 tasks

dapplion reviewed Nov 29, 2019

View reviewed changes

packages/aragon-cli/src/commands/ipfs_cmds/start.js Show resolved Hide resolved

dapplion reviewed Nov 29, 2019

View reviewed changes

kernelwhisperer force-pushed the refactor/ipfs-cmds-2nd-try branch from e20a4f8 to c5e499c Compare December 2, 2019 11:35

kernelwhisperer force-pushed the refactor/ipfs-cmds-2nd-try branch from c5e499c to ded0e39 Compare December 2, 2019 11:37

kernelwhisperer requested a review from macor161 December 2, 2019 11:38

theethernaut mentioned this pull request Dec 3, 2019

Refactor codebase for robustness (simplified) #981

Closed

26 tasks

0xGabi suggested changes Dec 3, 2019

View reviewed changes

Refactor IPFS cmds (again)

81d861b

kernelwhisperer force-pushed the refactor/ipfs-cmds-2nd-try branch from 023a1f5 to 81d861b Compare December 5, 2019 12:03

Fix code comments

5f734c7

kernelwhisperer changed the title ~~Fix ipfs start detached flag~~ Refactor IPFS cmds Dec 5, 2019

0xGabi self-requested a review December 5, 2019 13:43

0xGabi approved these changes Dec 5, 2019

View reviewed changes

0xGabi merged commit b63c6e1 into develop Dec 5, 2019

delete-merged-branch bot deleted the refactor/ipfs-cmds-2nd-try branch December 5, 2019 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor IPFS cmds #951

Refactor IPFS cmds #951

kernelwhisperer commented Nov 23, 2019 •

edited

Loading

codecov bot commented Nov 23, 2019 •

edited

Loading

dapplion commented Nov 24, 2019

dapplion Nov 29, 2019

kernelwhisperer Dec 1, 2019

dapplion commented Nov 29, 2019

kernelwhisperer commented Dec 1, 2019

dapplion commented Dec 1, 2019

macor161 commented Dec 2, 2019

dapplion commented Dec 2, 2019

kernelwhisperer commented Dec 2, 2019

kernelwhisperer commented Dec 2, 2019

dapplion commented Dec 2, 2019

kernelwhisperer commented Dec 3, 2019

0xGabi left a comment

0xGabi commented Dec 4, 2019

0xGabi left a comment

Refactor IPFS cmds #951

Refactor IPFS cmds #951

Conversation

kernelwhisperer commented Nov 23, 2019 • edited Loading

🦅 Pull Request

🚨 Test instructions

✔️ PR Todo

Imported issues from #952

ipfs data ✔️

ipfs daemon ✔️ :

codecov bot commented Nov 23, 2019 • edited Loading

Codecov Report

dapplion commented Nov 24, 2019

dapplion Nov 29, 2019

Choose a reason for hiding this comment

kernelwhisperer Dec 1, 2019

Choose a reason for hiding this comment

dapplion commented Nov 29, 2019

kernelwhisperer commented Dec 1, 2019

dapplion commented Dec 1, 2019

macor161 commented Dec 2, 2019

dapplion commented Dec 2, 2019

kernelwhisperer commented Dec 2, 2019

kernelwhisperer commented Dec 2, 2019

dapplion commented Dec 2, 2019

kernelwhisperer commented Dec 3, 2019

0xGabi left a comment

Choose a reason for hiding this comment

0xGabi commented Dec 4, 2019

0xGabi left a comment

Choose a reason for hiding this comment

kernelwhisperer commented Nov 23, 2019 •

edited

Loading

codecov bot commented Nov 23, 2019 •

edited

Loading