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

restruct config initialization #146

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

dai1975
Copy link
Contributor

@dai1975 dai1975 commented Aug 13, 2024

change config initialization structure to use logger in chain and/or prover init

before:

 PersistentPreRunE
 +-- InitConfig
 |   +-- UnmarshalJSON
 |   |   +-- json.Unmarshal
 |   |   +-- config.Chains[].Init
 |   |   +-- config.chains = config.Chains[].Build
 |   +-- InitChains
 |      +-- config.chains[].Init
 +-- initLogger

after:

 PersistentPreRunE
 +-- UnmarshalConfig
 |   +-- json.Unmarshal
 +-- initLogger
 +-- InitConfig
 |   +-- config.Chains[].Init
 |   +-- config.chains = config.Chains[].Build
 |   +-- initChains
 |      +-- config.chains[].Init

@dai1975 dai1975 requested a review from a team as a code owner August 13, 2024 09:22
@@ -128,7 +128,7 @@ func (c *Config) ChainsFromPath(path string) (map[string]*core.ProvableChain, st
}

// Called to initialize the relayer.Chain types on Config
func InitChains(ctx *Context, homePath string, debug bool) error {
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 function called by only this file.

return nil
}

func (ctx *Context) InitConfig(homePath string, debug bool) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old function signature (c *Config) InitConfig(ctx *Context, ...) is a bit confusing that c and ctx.Config should be same pointer or not.

Copy link
Contributor

@siburu siburu left a comment

Choose a reason for hiding this comment

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

@dai1975 Great work overall, thank you! I've left some comments in the code. Could you please review them and make the necessary changes?

config/config.go Outdated Show resolved Hide resolved
config/marshaler.go Outdated Show resolved Hide resolved
provers/mock/prover.go Outdated Show resolved Hide resolved
Signed-off-by: Daisuke Kanda <[email protected]>
@dai1975 dai1975 force-pushed the features/init-logger branch from eeaed5a to 810fd56 Compare August 15, 2024 12:10
Copy link
Contributor

@siburu siburu left a comment

Choose a reason for hiding this comment

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

@dai1975 LGTM. Thank you!!

@siburu siburu merged commit 4ecddcb into hyperledger-labs:main Aug 15, 2024
6 checks passed
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