-
Notifications
You must be signed in to change notification settings - Fork 44
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
feat(cmd): add index create subcommand to create an external carv2 index #350
Conversation
I'm in favor of this. the CLI commands are really just a grab bag of things we've found useful through development. at some point we should probably go through and clean it up with an eye towards what users would actually want now. That said, this seems like the right place to add this functionality. If you add a basic test I'm happy 👍 |
👌 simple test for this shouldn't be too hard |
car index create ${INPUTS}/sample-v1.car sample-v1.car.idx | ||
car detach-index ${INPUTS}/sample-wrapped-v2.car sample-wrapped-v2.car.idx | ||
cmp sample-v1.car.idx sample-wrapped-v2.car.idx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lmk if this is too simple and I can either add an index to the inputs to compare against or add something like:
car detach-index list sample-v1.car.idx
cmp stdout index-list.txt
-- index-list.txt --
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I think this is fine; we're not exactly doing sophisticated and high-coverage tests here in cmd
this does make me think though that maybe detach
should be moved to a subcmd of index
too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does make me think though that maybe detach should be moved to a subcmd of index too
I agree, but figured I'd let the maintainers make that call and if they'd be ok with a breaking move or want both supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't exactly have an SLA on this, cmd is currently just a grab-bag of utils we collectively find useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM. Happy to submit a PR to move it once this one is closed out
macos on 0.18 has been flaky for a little while now, unrelated to this change. gtg |
…-index feat(cmd): add index create subcommand to create an external carv2 index This commit was moved from ipld/go-car@31cce71
Ran into a case where I had a large CARv1 and wanted to create an index from it so I could use the two as a CARv2 blockstore (trying to take a 120GB ~69M block CARv1 of Filecoin state and use it as a blockstore for queries). IIUC this separation of the CARv1 from the index is common in Lotus/Boost and is part of why there's already a
detach
command.First time contributing to this CLI so lmk if there's a better way you had in mind, or a better place to put this command. We may also want an
index attach
command at some point, but that can be a separate PR.If people are happy with this, I can add a basic test for this as well (once I figure out the syntax of the test framework).