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.

Sign up for GitHub

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

Jump to bottom

new config adapter #6925

Merged
merged 15 commits into from
Aug 11, 2022
Merged

new config adapter #6925

merged 15 commits into from
Aug 11, 2022

Conversation

jmank88
Copy link
Contributor

@jmank88 jmank88 commented Jul 6, 2022 •
edited

@jmank88 jmank88 added the wip label Jul 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

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

jmank88
jmank88 commented Jul 6, 2022
View reviewed changes
internal/config/docs.toml Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022 •
edited

EVM Smoke Test Results

52 tests   22 ✔️  6m 37s ⏱️
  1 suites  30 💤
  1 files      0 ❌

Results for commit 39179e4.

♻️ This comment has been updated with latest results.

@jmank88 jmank88 force-pushed the sc-33615-new-config-impl branch 4 times, most recently from a28aeed to c7473ec Compare July 12, 2022 12:58
@jmank88 jmank88 mentioned this pull request Jul 14, 2022
Merged
@jmank88 jmank88 force-pushed the sc-33615-new-config-impl branch 4 times, most recently from 38c61f2 to 11d3501 Compare July 19, 2022 00:54
bolekk
bolekk reviewed Jul 19, 2022
View reviewed changes
core/config/v2/env.go Show resolved Hide resolved
core/services/chainlink/config_legacy.go Outdated Show resolved Hide resolved
This was referenced Jul 19, 2022
Merged
Merged
Merged
@jmank88 jmank88 force-pushed the sc-33615-new-config-impl branch 2 times, most recently from c2a0681 to 0943de1 Compare July 19, 2022 17:35
bolekk
bolekk reviewed Jul 19, 2022
View reviewed changes
Copy link
Contributor

@bolekk bolekk left a comment

Choose a reason for hiding this comment

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

A few more comments/questions. Apologies if they don't make sense :)

core/services/chainlink/config_legacy.go Outdated Show resolved Hide resolved
core/services/chainlink/config.go Outdated Show resolved Hide resolved
core/cmd/app.go Show resolved Hide resolved
bolekk
bolekk reviewed Jul 21, 2022
View reviewed changes
Copy link
Contributor

@bolekk bolekk left a comment

Choose a reason for hiding this comment

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

In general this PR looks good to me!

core/services/chainlink/config_general.go Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Jul 26, 2022 •
edited

Solana Smoke Test Results

1 tests   1 ✔️  4m 28s ⏱️
1 suites  0 💤
1 files    0 ❌

Results for commit 39179e4.

♻️ This comment has been updated with latest results.

@jmank88 jmank88 force-pushed the sc-33615-new-config-impl branch 2 times, most recently from 4916351 to a60f48a Compare July 26, 2022 13:43
@jmank88 jmank88 changed the title WIP: new config adapter new config adapter Jul 26, 2022
@jmank88 jmank88 force-pushed the sc-33615-new-config-impl branch 2 times, most recently from 8882a7c to 3938cbe Compare July 26, 2022 14:23
@jmank88 jmank88 force-pushed the sc-33615-new-config-impl branch 3 times, most recently from e3d1449 to b0728b9 Compare August 9, 2022 15:26
@jmank88 jmank88 requested a review from bolekk August 9, 2022 17:58
samsondav
samsondav requested changes Aug 9, 2022
View reviewed changes
core/chains/evm/config/v2/config.go
Comment on lines +381 to +383
if n.Name == nil {
err = multierr.Append(err, v2.ErrMissing{Name: "Name", Msg: "required for all nodes"})
} else if *n.Name == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the different scenarios where this could be nil or empty?

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 just user input, so when a user omits them or enters empty string.

internal/config/docs.toml
#
# Because of the complications with advisory locks, Chainlink nodes with v1.1.0 and later support a new `lease` locking mode. This mode might become the default in future. The `lease` locking mode works using the following process:
# Because of the complications with advisory locks, Chainlink nodes with v2.0 and later only support `lease` locking mode. The `lease` locking mode works using the following process:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't remove advisory locking completely, I think we just want to make lease the new default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you change your mind, or did I misunderstand? https://app.shortcut.com/chainlinklabs/story/45329/deprecate-database-locking-mode-advisory

bolekk
bolekk previously approved these changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@bolekk bolekk left a comment

Choose a reason for hiding this comment

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

The high-level structure looks good to me. I can't speak for specific config fields, though.

Given that this is not going to be in prod just yet, I don't see a large risk in merging, which will unblock a few smaller efforts that can be worked on in parallel.

@jmank88 jmank88 added the ready for review PR is ready for code review label Aug 10, 2022
@jmank88 jmank88 merged commit c1ccb03 into develop Aug 11, 2022
@jmank88 jmank88 deleted the sc-33615-new-config-impl branch August 11, 2022 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samsondav samsondav samsondav approved these changes

@pinebit pinebit pinebit approved these changes

@jkongie jkongie Awaiting requested review from jkongie

@bolekk bolekk Awaiting requested review from bolekk

Assignees
No one assigned
Labels
ready for review PR is ready for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jmank88 @bolekk @samsondav @pinebit