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(jstz): use file wrapper for kernel debug log in config #735

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

huancheng-trili
Copy link
Collaborator

@huancheng-trili huancheng-trili commented Jan 3, 2025

Context

Part of JSTZ-250.
JSTZ-250

Description

Use FileWrapper in jstz node config for kernel debug log.

kernel_debug_file does not get deserialised since we don't have any use case for that, so we don't need to implement Deserialize for FileWrapper. Overall, carrying a file like this in config is definitely not good because it complicates de/serialisation. There should be a refactor later on to get rid of all these whatever wrapper in config types and instead hold them somewhere else like in the jstzd state or so.

Manually testing the PR

  • Unit test: added one tests. All existing relevant tests should still work since fundamentally there is nothing changed.

@huancheng-trili huancheng-trili force-pushed the huanchengchang-jstz-250-2 branch 2 times, most recently from 30089c6 to 3fb7ccb Compare January 3, 2025 17:26
@huancheng-trili huancheng-trili changed the title feat(jstz_node): use file wrapper for kernel debug log in config feat(jstz): use file wrapper for kernel debug log in config Jan 3, 2025
@huancheng-trili huancheng-trili force-pushed the huanchengchang-jstz-250-2 branch from 3fb7ccb to d9ed6b7 Compare January 3, 2025 17:44
Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Project coverage is 47.80%. Comparing base (bd1b922) to head (91a67af).

Files with missing lines Patch % Lines
crates/jstzd/src/task/octez_rollup.rs 83.33% 0 Missing and 2 partials ⚠️
Files with missing lines Coverage Δ
crates/jstzd/src/config.rs 97.36% <100.00%> (-0.06%) ⬇️
crates/octez/src/async/rollup.rs 89.04% <100.00%> (+0.21%) ⬆️
crates/jstzd/src/task/octez_rollup.rs 79.62% <83.33%> (+0.46%) ⬆️

Continue to review full report in Codecov by Sentry.

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

@huancheng-trili huancheng-trili force-pushed the huanchengchang-jstz-250-2 branch from d9ed6b7 to 1cec355 Compare January 6, 2025 09:33
@huancheng-trili huancheng-trili marked this pull request as ready for review January 7, 2025 10:08
@huancheng-trili huancheng-trili force-pushed the huanchengchang-jstz-250-1 branch from 6653173 to 4fc3647 Compare January 8, 2025 09:47
Base automatically changed from huanchengchang-jstz-250-1 to main January 8, 2025 10:01
@huancheng-trili huancheng-trili force-pushed the huanchengchang-jstz-250-2 branch from 1cec355 to 91a67af Compare January 8, 2025 11:18
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.

2 participants