Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Basic UnixFS sanity checks #10701

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

PsychoPunkSage
Copy link

@PsychoPunkSage PsychoPunkSage commented Feb 6, 2025

Changes:

  • Basic Sanity checks for UnixFS nodes. <Raw data and dag-pb>
  • Added no-validate flag to cmd.options

Closes #10331

@PsychoPunkSage PsychoPunkSage requested a review from a team as a code owner February 6, 2025 05:31
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

triage note:

  • thank you, this is useful, but needs regression test
  • @PsychoPunkSage mind adding a test that confirms error occurs when someone tries to copy dag-cbor to MFS? (e.g. empty dag-cbor: bafyreigbtj4x7ip5legnfznufuopl4sg4knzc2cof6duas4b3q2fy6swua)

Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

See suggested changes and get CI passing first.

Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
@PsychoPunkSage
Copy link
Author

PsychoPunkSage commented Feb 15, 2025

Hi @lidel I wrote a test and it is failing ig... currently solving it... Can give some suggestions which might help me

=== RUN   TestFilesCp_DagCborNodeFails
    /home/psychopunk_sage/dev/OpenSource/kubo/core/commands/file_test.go:55: expected error but got nil
--- FAIL: TestFilesCp_DagCborNodeFails (0.01s)
FAIL
FAIL    github.com/ipfs/kubo/core/commands      0.051s

@gammazero Made required changes....

Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
@gammazero
Copy link
Contributor

gammazero commented Feb 18, 2025

The test is failing because the type assertion for dag.RawNode succeeds, here.

To test your code, the test needs to create something that is not a dag.RawNode and is also not a dag.ProtoNode or is a dag.ProtoNode that fails when calling FSNodeFromBytes on node.(*dag.ProtoNode).Data()

@gammazero gammazero added the need/author-input Needs input from the original author label Feb 18, 2025
Signed-off-by: Abhinav Prakash <[email protected]>
@PsychoPunkSage
Copy link
Author

Hi @gammazero
Thank you for help....

The Test is passing
Output

=== RUN   TestFilesCp_DagCborNodeFails
--- PASS: TestFilesCp_DagCborNodeFails (0.00s)
PASS
ok      github.com/ipfs/kubo/core/commands      0.029s

This is necessary because a "force" option is already used elsewhere, and its use in multiple places is causing CI to report the error: `Error: option name "force" used multiple times`
@gammazero gammazero added skip/changelog This change does NOT require a changelog entry and removed need/author-input Needs input from the original author labels Feb 20, 2025
@gammazero
Copy link
Contributor

@PsychoPunkSage I made 3 additional minor changes. See my 3 commits above.

Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

LGTM

@PsychoPunkSage
Copy link
Author

Hi @gammazero
I have gone through the changes... Thanks
GTG

@PsychoPunkSage
Copy link
Author

hi @lidel I think is PR is ready to be merged.
Please have a look at it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does NOT require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add basic UnixFS sanity check to 'ipfs files cp'
4 participants