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

Mx 14997 overwrite config structs #5851

Merged
merged 9 commits into from
Feb 1, 2024

Conversation

axenteoctavian
Copy link
Contributor

@axenteoctavian axenteoctavian commented Jan 18, 2024

Reasoning behind the pull request

  • Added the possibility to overwrite structs from config toml files

Proposed changes

  • changed from string to interface{}
  • implemented slice and struct cases
  • more checks (e.g. integer: new value can fit inside old value) - the toml will read numbers as int, but the actual config objects can be int8, uint8, int16 and so on

Testing procedure

  • real test files and structures created used in unit tests
  • running the node (overwrite operation happens at start-up)
  • *below change only tested locally, not included in PR files
    image

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

Copy link

codecov bot commented Jan 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e2a3c3d) 80.32% compared to head (4867e64) 80.39%.

Additional details and impacted files
@@                Coverage Diff                @@
##           rc/v1.7.next1    #5851      +/-   ##
=================================================
+ Coverage          80.32%   80.39%   +0.07%     
=================================================
  Files                710      710              
  Lines              94069    94222     +153     
=================================================
+ Hits               75563    75753     +190     
+ Misses             13222    13189      -33     
+ Partials            5284     5280       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@iulianpascalau iulianpascalau left a comment

Choose a reason for hiding this comment

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

Very good stuff. Good job!

common/reflectcommon/structFieldsUpdate_test.go Outdated Show resolved Hide resolved
common/reflectcommon/structFieldsUpdate_test.go Outdated Show resolved Hide resolved
testscommon/toml/overwriteConfig.go Show resolved Hide resolved
{ File = "config.toml", Path = "TestConfigStruct.ConfigStruct.Description", Value = { Nr = 222 } },
{ File = "config.toml", Path = "TestConfigStruct.ConfigStruct.Description", Value = { Number = "11" } },
{ File = "config.toml", Path = "TestConfigNestedStruct.ConfigNestedStruct", Value = { Text = "Overwritten text", Message = { Public = false, MessageDescription = [{ Text = "Overwritten Text1" }] } } },
{ File = "config.toml", Path = "TestConfigNestedStruct.ConfigNestedStruct.Message.MessageDescription", Value = [{ Text = "Overwritten Text1" }, { Text = "Overwritten Text2" }] },
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add an example like this on the original prefs.toml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already presented in prefs.toml but it's commented out.
This will be used later (we need this for some override in sovereign)

Copy link
Contributor

Choose a reason for hiding this comment

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

This feature is useful even if the sovereign is not finalized. A good example here is to override in prefs.toml file some settings regarding the HostDriverConfig from the external.toml file. Can we add a comment that makes a good example of how to override the Enabled & URL from that slice from external.toml file?

testscommon/toml/config.go Show resolved Hide resolved
common/reflectcommon/structFieldsUpdate_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@sstanculeanu sstanculeanu left a comment

Choose a reason for hiding this comment

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

good job with the unittests

common/reflectcommon/export_test.go Show resolved Hide resolved
sstanculeanu
sstanculeanu previously approved these changes Jan 26, 2024
Copy link
Contributor

@iulianpascalau iulianpascalau left a comment

Choose a reason for hiding this comment

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

Just the comment regarding the example in prefs.toml file.
We will retarget this PR to a release candidate branch.

iulianpascalau
iulianpascalau previously approved these changes Jan 31, 2024
@axenteoctavian axenteoctavian changed the base branch from master to rc/v1.7.next1 January 31, 2024 11:20
@axenteoctavian axenteoctavian dismissed iulianpascalau’s stale review January 31, 2024 11:20

The base branch was changed.

@axenteoctavian axenteoctavian marked this pull request as ready for review January 31, 2024 12:15
Copy link
Collaborator

@gabi-vuls gabi-vuls left a comment

Choose a reason for hiding this comment

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

Normal allin test: master-06f147d698 -> MX-14997-overwrite-config--4867e64b04

--- Specific errors ---

block hash does not match 5407
wrong nonce in block 2131
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 0

/------/

--- Statistics ---

Nr. of all ERRORS: 0
Nr. of all WARNS: 399
Nr. of new ERRORS: 0
Nr. of new WARNS: 2
Nr. of PANICS: 0

/------/

--- ERRORS ---

/------/

--- WARNINGS ---

/------/

@axenteoctavian axenteoctavian merged commit b4ce496 into rc/v1.7.next1 Feb 1, 2024
10 checks passed
@axenteoctavian axenteoctavian deleted the MX-14997-overwrite-config-structs branch February 1, 2024 18:53
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.

4 participants