Adding ipfs trait by BoisterousCoder · Pull Request #10 · fission-codes/fission-cli

BoisterousCoder · 2022-11-17T22:03:49Z

Description

This pull request implement an implantation for ipfs so code can later the call the following features:

Add IPFS trait
Add http module
Add ipfs drop trait to help with easy cleanup
Add ability to do swarm connect
~~Add ability to do swarm disconnect~~ Implicit disconnect when daemon closes
Add ability to edit config
~~Add ability to do bootstap add~~ This is covered by the config command
Add ability to upload files
Add ability to upload folders

Link to issue

closes #7

Testing

There are intregration test that are located in ./ipfs/tests.rs. Just do cargo test to run them and cargo test -- --nocapture if you need to see the console log. These test should hit the entire code. Just note these tests reference the fission production ipfs nodes and a couple of the tests upload files. Also you will need to have ipfs in your PATH as the daemon is launched using the ipfs daemon command.

Type of change

New feature (non-breaking change that adds functionality)
Comments have been added/updates

I am creating this new branch and moving my changes over, so I don't have to deal commit merging issues

We can connect! YAY

I am creating this new branch and moving my changes over, so I don't have to deal commit merging issues

We can connect! YAY

BoisterousCoder · 2022-12-23T21:12:33Z

I think its ready for review again but I don't dont think anyone is going to be able to review it until the 3rd

the changes I made to to my fork were merged with master

bgins

Made a few copy edit suggestions and comments on coding style while doing a first pass through the code.

Co-authored-by: Brian Ginsburg <7957636+bgins@users.noreply.github.com> Signed-off-by: Savannah Jackson <savannah@fission.codes>

bgins

I was researching how other projects organize test examples. What would you think about renaming test-dir to tests and then having a subdirectory named data where the test examples go?

It looks like that is how ripgrep has things organized: https://github.com/BurntSushi/ripgrep/tree/master/tests/data

bgins · 2023-01-05T19:46:39Z

+    // for peer in PEER_ADDRS {
+    //     block_on(ipfs.connect_to(peer)).unwrap();
+    //     println!("Connected to peer! {}", peer);
+    // }


Oh actually, this is in a test. Verbose mode doesn't matter here. 😅

I'd say lets keep the code or remove it depending on the feedback we want when running the test.

zeeshanlakhani

Adding a few bit of comments/questions now @BoisterousCoder. I'll go through this more.

zeeshanlakhani · 2023-01-09T21:48:23Z

+    async fn set_config(&self, property:&str, val:&Value) -> Result<()>;
+    /// This method returns the value a property in the IPFS config
+    /// 
+    /// Ex. `ipfs.get_config("Datastore.StorageMax")`


minor: There are specific standards for Rust "example" documentation: https://doc.rust-lang.org/beta/rust-by-example/meta/doc.html.

So, doc tests will fail if I attempt to do them properly because it gets a connection refused error. I can either remove those examples completely, find a way to do the doc tests without having to run them when cargo test is run, or leave them how they are.

fwiw doctests can be made ignore run with ignore keyword after the three backticks that assume "run rust"
If one wants to gate the doctest to make the ignore based on e.g. cfg() you can use cfg_attr like this:
https://github.com/dalek-cryptography/ed25519-dalek/blob/release/2.0/src/lib.rs#L21-22
Probably should improve that rust-by-example doc to include those keywords and how to use w/ attr
That also ponders the question if there is / could be a worthwhile mock like what httpmock provides in http world - or a record/replay artifact a'la fashion of insta.

Changed it to be an ignored doc test

zeeshanlakhani · 2023-01-09T21:50:21Z

+use crate::utils::file_management;
+
+
+fn run_ipfs_test<T>(test: T) -> ()


We should add a TODO on testing here, as most of this will be redone separately, as to not have to rely on running ipfs directly. Though, we should move all of these into an integration test setup, as in the template for example.

zeeshanlakhani · 2023-01-09T21:58:19Z

+use colored::Colorize;
+use walkdir::WalkDir;
+
+pub fn get_files_in(dir:&str) -> Result<HashMap<String, Vec<u8>>> {


I see this in the test, but do we need this anymore since we moved to the rust-api-client lib? get_files_in is done for add, right, but their add_path should already do the walking + adding work, right? https://github.com/ferristseng/rust-ipfs-api/blob/master/ipfs-api-prelude/src/api.rs#L205

I use that function in the testing so I can double check that all files have been uploaded. I think it won't be needed once we move to wire mock. However the current way I handle checking that all the files got uploaded is looking at the all the file and folder that should have been uploaded and checking that they are included in the return from the ipfs node.

zeeshanlakhani · 2023-01-09T22:13:02Z

+/// Ipfs often returns json with a single property with the value being an array. 
+/// This function simply takes the array in that property and turns it into a vector that can be used in rust.
+/// It will return an error result if the json is not formatted in this way.
+pub fn value_to_vec<A>(json:&Value, index:&str) -> Result<Vec<A>> 


I'd just remove this file :).

bgins

Looks good! 🎉

One small additional request around the organization of test files. Could we have them all inside test/data/ without the subdirectories? Aside from that, this is looking great!

BoisterousCoder · 2023-01-12T18:05:03Z

@bgins Those sub-directories are there to test that you can upload sub-directories and what's inside of them

bgins · 2023-01-12T19:29:44Z

@bgins Those sub-directories are there to test that you can upload sub-directories and what's inside of them

Ah ok, I missed that. Makes sense to keep them then. 👌

zeeshanlakhani

@BoisterousCoder Thanks for making all the changes, and this is in good shape! However, personally, I'd probably remove the dead code (as we can revisit things from utils at another point) and remove the tests using this code.

bgins · 2023-01-17T23:07:06Z

@BoisterousCoder Thanks for making all the changes, and this is in good shape! However, personally, I'd probably remove the dead code (as we can revisit things from utils at another point) and remove the tests using this code.

Yep, this seems like the right call. It will make the code base cleaner and easier to read.

We may have use for some of the dead code in the future. If we do, we can pull it up when we need it. I'll keep this branch around for future reference.

BoisterousCoder · 2023-01-18T02:27:30Z

@BoisterousCoder Thanks for making all the changes, and this is in good shape! However, personally, I'd probably remove the dead code (as we can revisit things from utils at another point) and remove the tests using this code.

Okay, dead code removed

zeeshanlakhani · 2023-01-18T23:58:52Z

+use colored::Colorize;
+use walkdir::WalkDir;
+
+pub fn get_files_in(dir: &str) -> Result<HashMap<String, Vec<u8>>> {


@BoisterousCoder minor: but is this needed still? I think the ipfs client add, adds dirs?

zeeshanlakhani

minus one question, 👍🏽

BoisterousCoder added 5 commits October 26, 2022 12:56

Moving stuff from other branch

cc7a1dd

I am creating this new branch and moving my changes over, so I don't have to deal commit merging issues

IPFS Trait Complete

3e5e770

adding what I got so I can review brian's pr

9bf1c5e

ready for testing for a lot of it

af8d470

first test done

9dc9831

We can connect! YAY

BoisterousCoder self-assigned this Nov 17, 2022

BoisterousCoder added the enhancement New feature or request label Nov 17, 2022

bgins mentioned this pull request Nov 17, 2022

Add IPFS trait and communication layer #7

Closed

BoisterousCoder added 22 commits November 18, 2022 14:32

fibonacci http retry wait

caaa869

try fail http fixed

3243116

refactor and more

cc4f983

files upload to ipfs finnally

faede32

removing auto disconnect for now

7c4d779

oops

c4442de

testing add file

a8114fe

finnishing add file

66c040e

quick fix for file adding

03bf7a4

remove warnings

72911e0

fixing config

31c0dee

Moving stuff from other branch

3750b4f

I am creating this new branch and moving my changes over, so I don't have to deal commit merging issues

IPFS Trait Complete

5c665be

adding what I got so I can review brian's pr

617cf35

ready for testing for a lot of it

a8024c2

first test done

3c06f28

We can connect! YAY

fibonacci http retry wait

bf7db06

try fail http fixed

0999da9

refactor and more

262c6e7

files upload to ipfs finnally

d27be60

removing auto disconnect for now

1660c8a

oops

5928dde

BoisterousCoder added 2 commits December 29, 2022 11:57

connect to

5243302

switch to main fork for ipfs-api

0758c6b

the changes I made to to my fork were merged with master

bgins reviewed Jan 4, 2023

View reviewed changes

Savannah Jackson and others added 6 commits January 4, 2023 16:04

Update src/ipfs.rs

b1b8126

Co-authored-by: Brian Ginsburg <7957636+bgins@users.noreply.github.com> Signed-off-by: Savannah Jackson <savannah@fission.codes>

Update src/ipfs.rs

88eea6c

Co-authored-by: Brian Ginsburg <7957636+bgins@users.noreply.github.com> Signed-off-by: Savannah Jackson <savannah@fission.codes>

Update src/ipfs.rs

09fae95

Co-authored-by: Brian Ginsburg <7957636+bgins@users.noreply.github.com> Signed-off-by: Savannah Jackson <savannah@fission.codes>

Update src/ipfs.rs

3f919c1

Co-authored-by: Brian Ginsburg <7957636+bgins@users.noreply.github.com> Signed-off-by: Savannah Jackson <savannah@fission.codes>

Update src/utils/config.rs

d3d5344

Co-authored-by: Brian Ginsburg <7957636+bgins@users.noreply.github.com> Signed-off-by: Savannah Jackson <savannah@fission.codes>

Update src/ipfs.rs

5c0bc5b

Co-authored-by: Brian Ginsburg <7957636+bgins@users.noreply.github.com> Signed-off-by: Savannah Jackson <savannah@fission.codes>

bgins reviewed Jan 4, 2023

View reviewed changes

bgins reviewed Jan 5, 2023

View reviewed changes

Comment thread src/utils/config.rs Outdated

Comment thread src/ipfs/daemon.rs

Comment thread src/ipfs/daemon.rs Outdated

bgins reviewed Jan 5, 2023

View reviewed changes

Comment thread src/ipfs/tests.rs Outdated

zeeshanlakhani suggested changes Jan 9, 2023

View reviewed changes

Savannah Jackson and others added 2 commits January 11, 2023 14:18

Accidently made changes to the wrong branch

831647f

mostly copy edits

e345e60

bgins approved these changes Jan 11, 2023

View reviewed changes

ignored doc tests

a3c934a

BoisterousCoder requested a review from zeeshanlakhani January 17, 2023 20:11

zeeshanlakhani reviewed Jan 17, 2023

View reviewed changes

Removing dead code

1a80100

BoisterousCoder requested a review from zeeshanlakhani January 18, 2023 23:55

zeeshanlakhani reviewed Jan 18, 2023

View reviewed changes

zeeshanlakhani approved these changes Jan 18, 2023

View reviewed changes

BoisterousCoder merged commit 5aafe82 into main Jan 19, 2023

		use crate::utils::file_management;


		fn run_ipfs_test<T>(test: T) -> ()

Uh oh!

Conversation

BoisterousCoder commented Nov 17, 2022 • edited by bgins Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Link to issue

Testing

Type of change

Uh oh!

BoisterousCoder commented Dec 23, 2022

Uh oh!

bgins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bgins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zeeshanlakhani left a comment

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!

pinkforest Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgins left a comment

Choose a reason for hiding this comment

Uh oh!

BoisterousCoder commented Jan 12, 2023

Uh oh!

bgins commented Jan 12, 2023

Uh oh!

zeeshanlakhani left a comment

Choose a reason for hiding this comment

Uh oh!

bgins commented Jan 17, 2023

Uh oh!

BoisterousCoder commented Jan 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeeshanlakhani left a comment

Choose a reason for hiding this comment

Uh oh!

BoisterousCoder commented Nov 17, 2022 •

edited by bgins

Loading

pinkforest Jan 12, 2023 •

edited

Loading