-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
support default config file overrides #14090
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
e269916
support default config file overrides
patrickhuie19 b4d777f
lint
patrickhuie19 f30500c
adding test for when default is not used
patrickhuie19 8a8919f
WIP: negative test for when an override is specified for a chain that…
patrickhuie19 d98d085
using env package
patrickhuie19 eab01f7
relaxing strict override restriction, and improving log line details
patrickhuie19 b77210b
lint + CustomDefaultsEnvKey --> CustomDefaults
patrickhuie19 3f9c8e8
removing invalid test
patrickhuie19 376c677
crash the node if can't read or load all files from overriden defaults
patrickhuie19 4bbfa50
accounting for Fatalf in control flow
patrickhuie19 a6c89f6
evm defaults look for evm suffix. Duplicate chain IDs fail validation
patrickhuie19 0a72af6
removing negative path testing due to CI failures
patrickhuie19 d045df6
removing mkdirs
patrickhuie19 e1d5521
updating config after rebase
patrickhuie19 ecdd101
re-enabling negative path
patrickhuie19 07d294e
removing comment
patrickhuie19 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong but these overrides are specified at runtime it seems? We're not statically embedding the overrides depending on a particular env var (seems hard / impossible now that I think about it since
go:embed
is on file level varsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this imply that these defaults need to be manually shipped to nops separately if they're not embedded in the built binary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but only if not using the official docker release which can bundle them in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree w/ @jmank88 - The easiest CX I can imagine for the NOPs is including the overrides directory and the env var in the docker image. That way they won't have to think about this at all.
It will take some time before CCIP doesn't release its own image, so this approach seems the easiest from a release perspective as well to me.