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

Do not use magic config file in integration tests #16441

Open
wants to merge 2 commits into
base: compatible
Choose a base branch
from

Conversation

dkijania
Copy link
Member

@dkijania dkijania commented Dec 18, 2024

Modifies code which formats entrypoint.sh file for deamon image which is used in integration tests through mina daemon puppeteer. It removes /var/lib/coda/config_{comit_hash}.json at the beginning as it is used as one of config layer by default. See https://github.com/MinaProtocol/mina/blob/compatible/src/lib/runtime_config/runtime_config.ml#L1728 for more details.

As a result, first layer of configs is config file from genesis_ledgers/{network}.json. In berkeley case it does not have any influence since berkeley config file has only content which is completely replaced by next layer (which is created by test executive). For other networks like devnet or mainnet it is more problematic as configs contains fork section, which then prevents node to produce any block. This is blocking us from using any other image (devnet for instance)

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania dkijania force-pushed the dkijania/do_not_use_magic_config_in_int_tests branch from e538f3b to 83711be Compare December 19, 2024 09:34
@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-nightly-me

@dkijania dkijania self-assigned this Dec 19, 2024
@dkijania dkijania marked this pull request as ready for review December 19, 2024 20:01
@dkijania dkijania requested a review from a team as a code owner December 19, 2024 20:01
( "entrypoint.sh"
, {|#!/bin/bash
# This file is auto-generated by the local integration test framework.
#
Copy link
Member

Choose a reason for hiding this comment

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

I wonder, could we just provide more overrides in integration test code instead of crude removal. P.S. in a med-term we want to get rid of this config discovery in a docker image.

Copy link
Member

Choose a reason for hiding this comment

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

I still suggest to explore the path of overriding the proof component in the integration test config instead of modification of a file below.

@dkijania dkijania force-pushed the dkijania/do_not_use_magic_config_in_int_tests branch from 9e0c3f1 to cd75cbd Compare January 6, 2025 14:42
@dkijania
Copy link
Member Author

dkijania commented Jan 6, 2025

!ci-build-me

@dkijania
Copy link
Member Author

dkijania commented Jan 6, 2025

!ci-nightly-me

@dkijania
Copy link
Member Author

dkijania commented Jan 6, 2025

( "entrypoint.sh"
, {|#!/bin/bash
# This file is auto-generated by the local integration test framework.
#
Copy link
Member

Choose a reason for hiding this comment

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

I still suggest to explore the path of overriding the proof component in the integration test config instead of modification of a file below.

@dkijania
Copy link
Member Author

dkijania commented Jan 8, 2025

Oh. I misunderstood you! Will work on altering config in int tests and keeping prod config intact

@georgeee
Copy link
Member

georgeee commented Jan 9, 2025

@dkijania please check b5537ab for implementation of the idea. I don't know if it will work, I didn't run integration tests on it, but at least that's what I meant originally.

@dkijania dkijania force-pushed the dkijania/do_not_use_magic_config_in_int_tests branch from cd75cbd to e71d6a7 Compare January 9, 2025 14:28
@dkijania
Copy link
Member Author

dkijania commented Jan 9, 2025

!ci-nightly-me

@dkijania
Copy link
Member Author

dkijania commented Jan 9, 2025

!ci-build-me

Copy link
Member

@georgeee georgeee left a comment

Choose a reason for hiding this comment

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

Please update the PR description

@dkijania
Copy link
Member Author

!ci-build-me

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