-
Notifications
You must be signed in to change notification settings - Fork 27
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 utility constructors for ReactiveComponentExecutionMode #2866
Conversation
979ce62
to
12aed6d
Compare
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.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @nadin-Starkware)
crates/starknet_sequencer_node/src/config/component_execution_config.rs
line 70 at r1 (raw file):
remote_server_config: None, } }
Call local_with_remote_disabled()
Code quote:
fn default() -> Self {
Self {
execution_mode: ReactiveComponentExecutionMode::LocalExecutionWithRemoteDisabled,
local_server_config: Some(LocalServerConfig::default()),
remote_client_config: None,
remote_server_config: None,
}
}
crates/starknet_sequencer_node/src/config/component_execution_config.rs
line 102 at r1 (raw file):
remote_server_config: Some(RemoteServerConfig { socket }), } }
Please move these to be under the testing
feature.
Code quote:
impl ReactiveComponentExecutionConfig {
pub fn disabled() -> Self {
Self {
execution_mode: ReactiveComponentExecutionMode::Disabled,
local_server_config: None,
remote_client_config: None,
remote_server_config: None,
}
}
pub fn remote(socket: SocketAddr) -> Self {
Self {
execution_mode: ReactiveComponentExecutionMode::Remote,
local_server_config: None,
remote_client_config: Some(RemoteClientConfig {
socket,
..RemoteClientConfig::default()
}),
remote_server_config: None,
}
}
pub fn local_with_remote_enabled(socket: SocketAddr) -> Self {
Self {
execution_mode: ReactiveComponentExecutionMode::LocalExecutionWithRemoteEnabled,
local_server_config: Some(LocalServerConfig::default()),
remote_client_config: None,
remote_server_config: Some(RemoteServerConfig { socket }),
}
}
6c22bb5
to
47ab72c
Compare
} | ||
|
||
#[cfg(any(feature = "testing", test))] | ||
async fn get_non_http_component_config(gateway_socket: SocketAddr) -> ComponentConfig { |
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 function needs to be marked as pub
since it's called by the public function get_remote_flow_test_config()
. Consider changing the function signature to pub async fn get_non_http_component_config
.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
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.
Done.
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.
:wat:
The bot's comment is wrong. Please ignore it, revert its request change, and dismiss its review if it's still blocking.
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.
I want to remove all these pub anyway, I saw its blocking
47ab72c
to
c004c33
Compare
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.
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @graphite-app[bot] and @Itay-Tsabary-Starkware)
crates/starknet_sequencer_node/src/config/component_execution_config.rs
line 70 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Call
local_with_remote_disabled()
Done.
crates/starknet_sequencer_node/src/config/component_execution_config.rs
line 102 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Please move these to be under the
testing
feature.
Done.
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.
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @graphite-app[bot] and @Itay-Tsabary-Starkware)
} | ||
|
||
#[cfg(any(feature = "testing", test))] | ||
async fn get_non_http_component_config(gateway_socket: SocketAddr) -> ComponentConfig { |
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.
Done.
c004c33
to
e367fbf
Compare
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.
Reviewed 3 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @graphite-app[bot] and @nadin-Starkware)
crates/starknet_integration_tests/Cargo.toml
line 9 at r2 (raw file):
[features] testing = []
Please remove this, see below.
Code quote:
[features]
testing = []
crates/starknet_integration_tests/src/end_to_end_integration.rs
line 1 at r2 (raw file):
#[cfg(any(feature = "testing", test))]
This module is not a library (i.e., other crates do not import from it).
It does not need to define features. Please remove these throughout.
Code quote:
#[cfg(any(feature = "testing", test))]
} | ||
|
||
#[cfg(any(feature = "testing", test))] | ||
async fn get_non_http_component_config(gateway_socket: SocketAddr) -> ComponentConfig { |
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.
:wat:
The bot's comment is wrong. Please ignore it, revert its request change, and dismiss its review if it's still blocking.
e367fbf
to
ed7bbb3
Compare
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.
Dismissed @graphite-app[bot] from a discussion.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)
crates/starknet_integration_tests/src/end_to_end_integration.rs
line 1 at r2 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
This module is not a library (i.e., other crates do not import from it).
It does not need to define features. Please remove these throughout.
Done.
} | ||
|
||
#[cfg(any(feature = "testing", test))] | ||
async fn get_non_http_component_config(gateway_socket: SocketAddr) -> ComponentConfig { |
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.
I want to remove all these pub anyway, I saw its blocking
commit-id:fe5285a8
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.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)
crates/starknet_integration_tests/Cargo.toml
line 9 at r2 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Please remove this, see below.
Done.
ed7bbb3
to
af74334
Compare
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.
Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nadin-Starkware)
✓ Commit merged in pull request #2867 |
Stack: