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

unify json config loading #16167

Open
wants to merge 1 commit into
base: martin/remove-phantom-config
Choose a base branch
from

Conversation

martyall
Copy link
Member

@martyall martyall commented Oct 1, 2024

This PR stacks on #16163

The Problem

There should only be one way to load json files to construct a value of type Runtime_config.t, we have several:

  • Mina_cli_entrypoint checks system directories for an installed config file, other places do not
  • Mina_cli_entrypoint logs verbosely where it finds certain values that belong the daemon object. This requires special handling to load these values only to gain a debug log -- we don't do this for any other fields or in any other CLI context, and would require more complicated handling. The fact that the ~keyname arguments for the logger are not derived from the value means they can drift.
  • Mina_cli_entrypoint will also attempt to write configuration as it is reading it. This addition was made something like 4 years ago. Rather than attempting to modify configuration on a users file system, at this point it seems wiser to just throw a parse exception.

The solution

  • Introduce a module Runtime_config.Json_loader to handle all Runtime_config loading logic, use this in all executables.
  • Use the same standard for loading daemon field values as we do for everything else
  • Remove the old update_config function that writes a json as it is loading.

@martyall martyall changed the base branch from compatible to martin/remove-phantom-config October 1, 2024 19:15
@martyall martyall force-pushed the martin/unify-json-config-loading branch from 463cdaa to 2f4b4bf Compare October 1, 2024 19:20
@martyall
Copy link
Member Author

martyall commented Oct 1, 2024

!ci-build-me

@martyall
Copy link
Member Author

martyall commented Oct 1, 2024

!ci-nightly-me

@martyall
Copy link
Member Author

martyall commented Oct 1, 2024

Nightly tests here https://buildkite.com/o-1-labs-2/mina-o-1-labs/builds/35530#_

Passes everything except for merge cleanly checks, which will wait to review and supporting PRs are merged

@martyall martyall marked this pull request as ready for review October 2, 2024 00:01
@martyall martyall requested a review from a team as a code owner October 2, 2024 00:01
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.

1 participant