-
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
Automation v2.1: fixes and leftovers #10599
Conversation
I see that you haven't updated any README files. Would it make sense to do so? |
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6159362571 |
Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6160167409 |
a92b080
to
a707c34
Compare
core/services/ocr2/plugins/ocr2keeper/evm21/block_subscriber.go
Outdated
Show resolved
Hide resolved
ctx, cancel := context.WithCancel(pctx) | ||
defer cancel() | ||
|
||
lggr := p.lggr.With("where", "reader") | ||
lggr := p.lggr.Named(fmt.Sprintf("ReaderThread-%d", tid)) |
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.
Presumably tid
is thread ID here? Could we perhaps rename the function to startReaderThread
?
@@ -70,7 +70,7 @@ func TestLogEventProvider_LifeCycle(t *testing.T) { | |||
}, | |||
{ | |||
"existing config with old block", | |||
true, | |||
false, |
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 don't expect you to change this, but maybe in future as a team we should strive to use field names against these values, just for more context on what they represent if anything, especially in code reviews
core/services/ocr2/plugins/ocr2keeper/evm21/logprovider/recoverer.go
Outdated
Show resolved
Hide resolved
@@ -175,7 +175,7 @@ func TestIntegration_LogEventProvider_UpdateConfig(t *testing.T) { | |||
TriggerConfig: cfg, | |||
UpdateBlock: bn.Uint64() - 1, | |||
}) | |||
require.Error(t, err) | |||
require.NoError(t, err) |
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.
this seems like the opposite assert now. not sure why this needs to change with the naming changes?
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.
We will log (debug) rather than returning an error, therefore the error check is irrelevant
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
No description provided.