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

chore: add disabled function to ComponentConfig #2867

Merged
merged 2 commits into from
Dec 23, 2024

Conversation

nadin-Starkware
Copy link
Collaborator

@nadin-Starkware nadin-Starkware commented Dec 22, 2024

commit-id:9673c1ca


Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a 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_integration_tests/src/config_utils.rs line 122 at r1 (raw file):

        ..ComponentConfig::default()
    }
}

Please format this function to be as get_http_only_component_config

Code quote:

pub async fn get_non_http_component_config(gateway_socket: SocketAddr) -> ComponentConfig {
    ComponentConfig {
        http_server: ActiveComponentExecutionConfig::disabled(),
        monitoring_endpoint: Default::default(),
        gateway: ReactiveComponentExecutionConfig::local_with_remote_enabled(gateway_socket),
        ..ComponentConfig::default()
    }
}

crates/starknet_integration_tests/src/config_utils.rs line 124 at r1 (raw file):

}

pub async fn get_remote_flow_test_config() -> Vec<ComponentConfig> {

Is this still for the flow test? If not, please rename.

Code quote:

get_remote_flow_test_config()

Copy link
Collaborator Author

@nadin-Starkware nadin-Starkware left a 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 3 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)


crates/starknet_integration_tests/src/config_utils.rs line 122 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please format this function to be as get_http_only_component_config

Default doesn't provide mutable fields for assignment after creation.


crates/starknet_integration_tests/src/config_utils.rs line 124 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Is this still for the flow test? If not, please rename.

Done.

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a 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 3 files reviewed, 2 unresolved discussions (waiting on @nadin-Starkware)


crates/starknet_integration_tests/src/config_utils.rs line 122 at r1 (raw file):

Previously, nadin-Starkware (Nadin Jbara) wrote…

Default doesn't provide mutable fields for assignment after creation.

let mut config = ComponentConfig::Default()
config.X = Y

That doesn't work?

Copy link
Collaborator Author

@nadin-Starkware nadin-Starkware left a 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 3 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)


crates/starknet_integration_tests/src/config_utils.rs line 122 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…
let mut config = ComponentConfig::Default()
config.X = Y

That doesn't work?

Nope

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r1, 1 of 1 files at r4, all commit messages.
Reviewable status: 2 of 3 files reviewed, all discussions resolved

Copy link
Collaborator Author

@nadin-Starkware nadin-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nadin-Starkware)

@nadin-Starkware nadin-Starkware changed the base branch from spr/main/fe5285a8 to main December 23, 2024 12:34
@nadin-Starkware nadin-Starkware merged commit 76a13c9 into main Dec 23, 2024
8 checks passed
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.

3 participants