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

Idan/main/add sequencer deployment tools #1856

Closed
wants to merge 16 commits into from

Conversation

idan-starkware
Copy link
Contributor

No description provided.

@idan-starkware idan-starkware self-assigned this Nov 6, 2024
@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.16%. Comparing base (e3165c4) to head (cfabecb).
Report is 418 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1856       +/-   ##
===========================================
+ Coverage   40.10%   77.16%   +37.05%     
===========================================
  Files          26      378      +352     
  Lines        1895    40027    +38132     
  Branches     1895    40027    +38132     
===========================================
+ Hits          760    30886    +30126     
- Misses       1100     6858     +5758     
- Partials       35     2283     +2248     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@idan-starkware idan-starkware force-pushed the idan/main/add-sequencer-deployment-tools branch from f316fa1 to d0a0608 Compare November 12, 2024 08:43
@idan-starkware idan-starkware force-pushed the idan/main/add-sequencer-deployment-tools branch from d0a0608 to 698cddd Compare November 12, 2024 09:45
Comment on lines +115 to +116
mount_path=pvc.mount_path,
read_only=True
Copy link

Choose a reason for hiding this comment

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

The read_only flag in the volume mount should match the corresponding PVC's read_only setting rather than being hardcoded to true. Consider updating to read_only=pvc.read_only to maintain consistency between the mount and PVC configurations.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@idan-starkware idan-starkware force-pushed the idan/main/add-sequencer-deployment-tools branch from 7a6e141 to cfbc596 Compare November 13, 2024 10:40
scope=app,
name="sequencer-node",
namespace="sequencer-node-test",
config=None
Copy link

Choose a reason for hiding this comment

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

The config parameter is marked as required in SequencerNode's implementation but is being passed as None here. This will cause runtime errors since SequencerNode uses the config object without null checks. Either make the parameter optional in SequencerNode and add appropriate null handling, or pass a valid config object at instantiation.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@idan-starkware idan-starkware force-pushed the idan/main/add-sequencer-deployment-tools branch from 6f0c4a4 to ccae39a Compare November 15, 2024 14:43
@github-actions github-actions bot locked and limited conversation to collaborators Nov 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants