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

[TT-1046] Add Chainlink node test config as TOML #13142

Merged
merged 47 commits into from
May 20, 2024

Conversation

Tofel
Copy link
Contributor

@Tofel Tofel commented May 8, 2024

What was done?

  • we can leverage existing TOML files that configure tests to pass CL node config
  • it supports chain-specific configuration, where configuration is selected based on chain id; if no match is found, then default is used
  • functional options used for programmatically configuring chainlink node TOML configuration were removed (so that we don't have to think about precedence of options vs TOML and about effectively communicating to the user which ones were used); functional options for CLNode were left untouched
  • because OTEL tracing required unique ids for each node that id is generated programmatically, when we detect that tracing is enabled
  • removed methods from builder that were not needed any more and renamed another one to better reflect what they do

⚠️ Disclaimer ⚠️
Chainlink nodes funding has been increased for some tests, due to default GasEstimator settings that's higher than it was in the past. If that's an issue, please let me know!

@Tofel Tofel marked this pull request as ready for review May 9, 2024 06:43
@Tofel Tofel requested review from a team as code owners May 9, 2024 06:43
anirudhwarrier
anirudhwarrier previously approved these changes May 16, 2024
lukaszcl
lukaszcl previously approved these changes May 16, 2024
iljapavlovs
iljapavlovs previously approved these changes May 16, 2024
@Tofel Tofel dismissed stale reviews from iljapavlovs, lukaszcl, and anirudhwarrier via b1811a1 May 17, 2024 07:31
@Tofel Tofel requested a review from jmank88 May 17, 2024 11:16

# Chainlink node TOML config
[NodeConfig]
BaseConfigTOML = """
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to nest this TOML? Depending on the TOML lib, I think it would be possible to use a type for this field which defers parsing (like json.RawMessage), so that we could handle it as dynamic TOML instead of an opaque string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do like this idea, but my only doubt is that it will work differently than it does now in CCIP and idk if it's worth diverting.

jmank88
jmank88 previously approved these changes May 20, 2024
@cl-sonarqube-production
Copy link

@Tofel Tofel added this pull request to the merge queue May 20, 2024
Merged via the queue into develop with commit 99e2a65 May 20, 2024
107 checks passed
@Tofel Tofel deleted the tt_1046_cl_node_test_config_as_toml branch May 20, 2024 12:47
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.

7 participants