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

add default l1 additional services #79

Closed
wants to merge 2 commits into from
Closed

Conversation

tedim52
Copy link
Contributor

@tedim52 tedim52 commented Apr 23, 2024

Description

Adds a default value for value added here: #74. Adding defaults in the run just makes it so the package is always runnable without needing to enter a params.yml , which is useful when running the package via the UI.

@leovct
Copy link
Member

leovct commented Apr 23, 2024

Thanks you @tedim52 :)

@leovct
Copy link
Member

leovct commented Apr 23, 2024

Description

Adds a default value for value added here: #74. Adding defaults in the run just makes it so the package is always runnable without needing to enter a params.yml , which is useful when running the package via the UI.

I had a question regarding #73, we used to run the permissionless node directly from zkevm_permissionless_node.star using the following command:

kurtosis run --enclave cdk-v1 --args-file params.yml --main-file zkevm_permissionless_node.star .

But this is now impossible because of the change made to params.yml.

INFO[2024-04-23T15:55:19+02:00] Executing Starlark package at '/Users/leovct/Documents/work/infra/kurtosis-cdk' as the passed argument '.' looks like a directory 
INFO[2024-04-23T15:55:19+02:00] Compressing package 'github.com/0xPolygon/kurtosis-cdk' at '.' for upload 
INFO[2024-04-23T15:55:26+02:00] Uploading and executing package 'github.com/0xPolygon/kurtosis-cdk' 
There was an error interpreting Starlark code 
Evaluation error: key "deployment_suffix" not in dict
	at [github.com/0xPolygon/kurtosis-cdk/zkevm_permissionless_node.star:9:40]: run

I was wondering if you had a solution for that? If not, we can set all those variables to false, except for deploy_zkevm_permissionless_node in params.yml.

# Deploy local L1.
deploy_l1: false

# Deploy zkevm contracts on L1 (and also fund accounts).
deploy_zkevm_contracts_on_l1: false

# Deploy zkevm node and cdk peripheral databases.
deploy_databases: false

# Deploy cdk central/trusted environment.
deploy_cdk_central_environment: false

# Deploy cdk/bridge infrastructure.
deploy_cdk_bridge_infra: false

# Deploy permissionless node.
deploy_zkevm_permissionless_node: true

# Deploy observability stack.
deploy_observability: false

@tedim52
Copy link
Contributor Author

tedim52 commented Apr 23, 2024

Ah yep, the current workaround would be to do as you said - add the deploy_xxx args to the run(plan, ...): function in zkevm_permissionless_node.star just as in main.star and set the appropriate values. This isn't great because the other deploy_xxx values have no effect for this starlark script so they shouldn't be parameterized in it.

Another solution would be to make a separate params file for the permissionless node, but then you'd have to maintain params in 2 place, ideally you could just use one params file for all your scripts in this package.

The ideal solution would be to adjust logic so that the run function of each Starlark script only picks up params that are in its function spec, and doesn't fail when others are provided in an args file. So in this case:

params.yml:

# Deploy local L1.
deploy_l1: false

.... other deploy params
args: 
   ...

zkevm_permissionless_node.star:

def run(plan, args):
     ... 
kurtosis run --enclave cdk-v1 --args-file params.yml --main-file zkevm_permissionless_node.star .

and the starlark script would only take the value from the args key in the params.yml, and ignore the other values, instead of failing as it would currently.

I've created an issue to track this here: kurtosis-tech/kurtosis#2398

@leovct
Copy link
Member

leovct commented Apr 24, 2024

@tedim52 Can you sign your commits? I know it's annoying 😅

@leovct leovct mentioned this pull request Apr 26, 2024
@leovct
Copy link
Member

leovct commented Apr 26, 2024

Closing in favour of #90

@leovct leovct closed this Apr 26, 2024
auto-merge was automatically disabled April 26, 2024 08:07

Pull request was closed

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