Config split #302
Merged
Config split #302
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lockchain overwrite
wrap BuildConfig into NewConfig being responsible to cfg validation making Config Unmarshalling testing easier
…rning info into another variable
najeal
requested review from
michaelkaplan13,
cam-schultz,
minghinmatthewlam,
geoff-vball and
bernard-avalabs
as code owners
geoff-vball
previously approved these changes
Jun 5, 2024
There was a problem hiding this comment.
Thanks for this! It overall looks good to me. @cam-schultz should also take a look at this
cam-schultz
reviewed
Jun 11, 2024
| var configFile string | ||
|
|
||
| func TestBuildConfig(t *testing.T) { | ||
| v := viper.New() |
There was a problem hiding this comment.
We can get rid of test_files/config.json and re-use the top-level sample-relayer-config.json for this test. The following changes are necessary:
- Read in
"../sample-relayer-config.jsonusingos.ReadFile.go:embeddoes not support...
cfgBytes, err := os.ReadFile("../sample-relayer-config.json")
configFile := string(cfgBytes)
- Modify the expected
DestinationBlockchainAccountPrivateKeyto match the partial key provided insample-relayer-config.json - Remote
test_files/config.json
Co-authored-by: cam-schultz <[email protected]> Signed-off-by: nathan haim <[email protected]>
cam-schultz
reviewed
Jun 11, 2024
| func TestBuildConfig(t *testing.T) { | ||
| v := viper.New() | ||
| cfgBytes, err := os.ReadFile("../sample-relayer-config.json") |
There was a problem hiding this comment.
Let's check the error here. This is also the cause of the failing lint job.
There was a problem hiding this comment.
Sorry my mind was elsewhere. It should be ok now
There was a problem hiding this comment.
No worries! Thanks for the fix.
geoff-vball
approved these changes
Jun 11, 2024
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why this should be merged
It is related to #251
This splits the config go file into multiple ones easier to read.
It shorten the BuildConfig function
It provides more informations about the config overwritten options.
How this works
Split does not change logic.
About
BuildConfig()changes:viper.UnmarshalKey(), a uniqueviper.Unmarshal()is done.NewConfig()wrapsBuildConfig()+config Validation. This makes the test of viper unmarshaling easier mechanism fromBuildConfig().Information details about overwritten options are now embedded in a Config private field. It can be accessed from new methods
HasOverwrittenOptions()&GetOverwrittenOptions()How this was tested
Modified unmarshal mechanism is tested in
viper_test.goHow is this documented