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

feat: add save_to_file option to cli state fetch #3148

Merged
merged 8 commits into from
Jul 14, 2023

Conversation

hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Jul 7, 2023

Summary of changes

As part of #3081

Changes introduced in this pull request:

  • Add an option to state fetch to save the DAG to a file

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@hanabi1224 hanabi1224 changed the title feat: add save_to_file option to cli sate fetch feat: add save_to_file option to cli state fetch Jul 7, 2023
@hanabi1224 hanabi1224 force-pushed the hm/state-fetch-save-to-file branch from 646f215 to b959b25 Compare July 7, 2023 14:02
@hanabi1224 hanabi1224 changed the title feat: add save_to_file option to cli state fetch feat: add save_to_file option to cli state fetch Jul 7, 2023
@hanabi1224 hanabi1224 force-pushed the hm/state-fetch-save-to-file branch 4 times, most recently from b562291 to 76eff57 Compare July 7, 2023 16:04
with:
go-version: "^1.20"
- name: Install go-car
run: go install github.com/ipld/go-car/cmd/car@v0.0.0-20230704002448-4de031a1d8a8
Copy link
Member

Choose a reason for hiding this comment

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

I like this versioning. Very agile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's recommended to use @latest locally but I would rather use a fixed version on CI

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was just mocking the 0.0.0 version. You cant' have any expectations in such a case. :)

Comment on lines 57 to 60
echo "Verifying saved .car file"
car verify bafy2bzacedjq7lc42qhlk2iymcpjlanntyzdupc3ckg66gkca6plfjs5m7euo.car
echo "Inspecting saved .car file"
car inspect bafy2bzacedjq7lc42qhlk2iymcpjlanntyzdupc3ckg66gkca6plfjs5m7euo.car
Copy link
Member

Choose a reason for hiding this comment

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

I'd put it into a separate file. The reason is, it needs a separate binary and I would like to be able to run ./scripts/tests/calibnet_other_checks.sh without it. Alternatively, I'm good with it if we could have it in a docker image (we'd avoid a Go dependency in the workflow this way)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved out of test script

run: |
# File name is hard-coded in ./scripts/tests/calibnet_other_check.sh
car verify bafy2bzacedjq7lc42qhlk2iymcpjlanntyzdupc3ckg66gkca6plfjs5m7euo.car
car inspect bafy2bzacedjq7lc42qhlk2iymcpjlanntyzdupc3ckg66gkca6plfjs5m7euo.car
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the value of car inspect? Can it fail even after car verify has succeeded? If it's only used for debugging then let's remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@hanabi1224 hanabi1224 added this pull request to the merge queue Jul 14, 2023
Merged via the queue into main with commit 6fcbd8a Jul 14, 2023
20 checks passed
@hanabi1224 hanabi1224 deleted the hm/state-fetch-save-to-file branch July 14, 2023 10:27
@hanabi1224 hanabi1224 mentioned this pull request Jul 17, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants