[BCF-2261] Break up config/v2 #9697

cedric-cordenier · 2023-06-22T17:17:10Z

This PR is a proposal to break up the config/v2 package. Before this PR, the package combined multiple responsibilities, namely:

providing an API to validate structs, via Validate(interface{}).
providing utilities for dealing with TOML structs, eg. a unified DecodeTOML method.
the concrete struct types for non-chain-specific config.
providing an API to deal with env variables.
providing a toolset to deal with automated doc generation for config settings.

This PR breaks up these responsibilities so that the config/v2 package (renamed config/toml) is only responsible for defining the structs for the non-chain-specific config.

Move config/env.go into its own package: config/env.
Move config utilities relating to TOML, ValidateConfig, into utils/configutils.
Rename config/v2 -> config/toml. This package now only contains the TOML structs for the base config.
Move config/v2/docs -> config/docs.
Stop an import cycle by placing the integration tests which verify defaults are correctly extracted for chain-specific configs into a separate docs_test package.

github-actions · 2023-06-22T17:17:34Z

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

jmank88 · 2023-06-23T12:58:36Z

core/utils/validate.go

@@ -23,7 +21,7 @@ type Validated interface {

 // Validate returns any errors from calling Validated.ValidateConfig on cfg and any nested types that implement Validated.
 func Validate(cfg interface{}) (err error) {


How about a more specific name if this is going to live in a general utils package?

Suggested change

func Validate(cfg interface{}) (err error) {

func ValidateConfig(cfg interface{}) (err error) {

If anything, I would suggest making it less, rather than more, specific. That way this can be used generally to validate any struct.

eg.

func Validate(s interface{}) error { ... } type Validated interface { IsValid() error }

But isn't the code hyper-specific to toml config? I'm concerned about moving any pieces over to utils:

Move config utilities relating to TOML, ValidateConfig, into utils.

It would not be appropriate to use most (all?) of these as-is from other parts of the system - TOML especially, since we use a different module here than for job specs, for example. I think they need to remain config scoped, at least by name, but package separation seems cleaner.

Hmmm... yes, fair point. How about a configutils package containing DecodeTOML, ValidateConfig and the various errors that might be emitted from ValidateConfig() functions?

What I want to achieve with this is to break the dependency between config/v2 and evmconfig/v2, since previously evmconfig/v2 would depend on config/v2, and this caused some import cycles, by moving the common parts out to a separate package.

Actually, in the future LOOP Plugins will be responsible for validating their own config, so we will want to move these generic pieces over to chainlink-relay to be available for other repos. With that in mind, I think it would be simplest to keep a specialized package in the meantime.

Hmmm... yes, fair point. How about a configutils package containing DecodeTOML, ValidateConfig and the various errors that might be emitted from ValidateConfig() functions?

Yeah I think that would work 👍

There is a minimal package config in chainlink-relay with copies of some error types:
https://github.com/smartcontractkit/chainlink-relay/tree/main/pkg/config

Understood! Would you like me to move these pieces to chainlink-relay and import them here? Or should I just stick to putting them in configutils for now?

I think a local package is fine for now. We can move it later when we move the chain family types for LOOPs.

jmank88 · 2023-06-23T16:13:34Z

core/chains/cosmos/config.go

@@ -17,37 +17,37 @@ import (

 	"github.com/smartcontractkit/chainlink/v2/core/chains"
 	"github.com/smartcontractkit/chainlink/v2/core/chains/cosmos/types"
-	v2 "github.com/smartcontractkit/chainlink/v2/core/config/v2"
+	"github.com/smartcontractkit/chainlink/v2/core/utils/configutils"


nit/ I think it would be more idiomatic to name the package just config to avoid repeating utils:

Suggested change

"github.com/smartcontractkit/chainlink/v2/core/utils/configutils"

"github.com/smartcontractkit/chainlink/v2/core/utils/config"

Can still name the imports when there are collisions 🤷

Suggested change

"github.com/smartcontractkit/chainlink/v2/core/utils/configutils"

configutils "github.com/smartcontractkit/chainlink/v2/core/utils/config"

jmank88 · 2023-06-23T16:15:59Z

core/config/docs/docs.go

@@ -38,12 +38,12 @@ var (
 	//go:embed example-secrets.toml
 	exampleSecrets string

-	docsTOML = coreTOML + chainsEVMTOML + chainsCosmosTOML + chainsSolanaTOML + chainsStarknetTOML
+	DocsTOML = CoreDefaultsTOML + chainsEVMTOML + chainsCosmosTOML + chainsSolanaTOML + chainsStarknetTOML


Is there any way to avoid exporting these vars? Ideally they would be co-located with their usage. IIRC there was an obstacle to using an internal/ directory when I first implemented this, but that may have changed.

core/config/env/env.go

cedric-cordenier · 2023-07-10T09:36:32Z

core/config/docs/defaults.go

+)
+
+func CoreDefaults() (c toml.Core) {
+	if (defaults == toml.Core{}) {


@jmank88 Let me know what you think of this change; instinctively I think it's an anti-pattern to have packages implement init, so I made the instantiation of defaults lazy. This shouldn't change any behaviour since CoreDefaults is still one of the very first calls made at startup.

How is this an improvement over an init func? A sync.Once would be more efficient than a mutex, but with the init you don't need to synchronize at all.

core/config/docs/docs.go

cedric-cordenier · 2023-07-10T12:54:43Z

@jmank88 I've removed my changes to CoreDefault and added the init back in. Your comment about that seems to have disappeared, but my thinking here wasn't primarily about efficiency, it was primarily about the principle of least surprise and avoiding side effects when a package is imported.

core/config/env/env.go

cl-sonarqube-production · 2023-07-10T15:04:24Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

83.8% Coverage
0.0% Duplication

cedric-cordenier force-pushed the bcf-2261-rename-config-v2 branch 5 times, most recently from 42de976 to e22764a Compare June 23, 2023 11:44

cedric-cordenier changed the title ~~Bcf 2261 rename config v2~~ [BCF-2261] Break up config/v2 Jun 23, 2023

cedric-cordenier marked this pull request as ready for review June 23, 2023 12:37

cedric-cordenier requested review from a team, jkongie, jmank88, samsondav, PiotrTrzpil, archseer, cfal and prashantkumar1982 as code owners June 23, 2023 12:37

jmank88 reviewed Jun 23, 2023

View reviewed changes

cedric-cordenier force-pushed the bcf-2261-rename-config-v2 branch 3 times, most recently from 5427476 to 3486897 Compare June 23, 2023 15:00

cedric-cordenier requested a review from jmank88 June 23, 2023 15:29

jmank88 reviewed Jun 23, 2023

View reviewed changes

core/config/env/env.go Outdated Show resolved Hide resolved

cedric-cordenier force-pushed the bcf-2261-rename-config-v2 branch 2 times, most recently from eb283c4 to 2642950 Compare June 23, 2023 17:13

cedric-cordenier added 3 commits July 10, 2023 10:04

[BCF-2261] Split up config/v2; rename config/v2 -> config/toml

34d161b

Rename Env -> Var; EnvSecret -> Secret

5051c75

utils/configutils -> utils/config

71edd75

cedric-cordenier force-pushed the bcf-2261-rename-config-v2 branch from 2642950 to 71edd75 Compare July 10, 2023 09:04

cedric-cordenier commented Jul 10, 2023

View reviewed changes

Make coreDefaultsTOML private again

830e674

cedric-cordenier force-pushed the bcf-2261-rename-config-v2 branch from 23ac82e to 830e674 Compare July 10, 2023 10:00

jmank88 reviewed Jul 10, 2023

View reviewed changes

core/config/docs/docs.go Outdated Show resolved Hide resolved

Address review comments

279e4ab

cedric-cordenier requested a review from jmank88 July 10, 2023 12:50

jmank88 reviewed Jul 10, 2023

View reviewed changes

core/config/env/env.go Outdated Show resolved Hide resolved

jmank88 previously approved these changes Jul 10, 2023

View reviewed changes

LooppHostName -> LOOPPHostName

42d13da

cedric-cordenier dismissed jmank88’s stale review via 42d13da July 10, 2023 14:07

Merge branch 'develop' into bcf-2261-rename-config-v2

f0b0f36

george-dorin approved these changes Jul 10, 2023

View reviewed changes

cedric-cordenier enabled auto-merge July 10, 2023 14:52

cedric-cordenier added this pull request to the merge queue Jul 10, 2023

Merged via the queue into develop with commit cef1712 Jul 10, 2023
95 checks passed

cedric-cordenier deleted the bcf-2261-rename-config-v2 branch July 10, 2023 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BCF-2261] Break up config/v2 #9697

[BCF-2261] Break up config/v2 #9697

cedric-cordenier commented Jun 22, 2023 •

edited

Loading

github-actions bot commented Jun 22, 2023

jmank88 Jun 23, 2023

cedric-cordenier Jun 23, 2023

jmank88 Jun 23, 2023

cedric-cordenier Jun 23, 2023

jmank88 Jun 23, 2023

jmank88 Jun 23, 2023

jmank88 Jun 23, 2023

cedric-cordenier Jun 23, 2023

jmank88 Jun 23, 2023

jmank88 Jun 23, 2023

jmank88 Jun 23, 2023

cedric-cordenier Jul 10, 2023

jmank88 Jul 10, 2023

cedric-cordenier commented Jul 10, 2023

cl-sonarqube-production bot commented Jul 10, 2023

		@@ -23,7 +21,7 @@ type Validated interface {

		// Validate returns any errors from calling Validated.ValidateConfig on cfg and any nested types that implement Validated.
		func Validate(cfg interface{}) (err error) {

	func Validate(cfg interface{}) (err error) {
	func ValidateConfig(cfg interface{}) (err error) {

	"github.com/smartcontractkit/chainlink/v2/core/utils/configutils"
	"github.com/smartcontractkit/chainlink/v2/core/utils/config"

	"github.com/smartcontractkit/chainlink/v2/core/utils/configutils"
	configutils "github.com/smartcontractkit/chainlink/v2/core/utils/config"

[BCF-2261] Break up config/v2 #9697

[BCF-2261] Break up config/v2 #9697

Conversation

cedric-cordenier commented Jun 22, 2023 • edited Loading

github-actions bot commented Jun 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedric-cordenier commented Jul 10, 2023

cl-sonarqube-production bot commented Jul 10, 2023

cedric-cordenier commented Jun 22, 2023 •

edited

Loading