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

Automation v2.1: fixes and leftovers #10599

Closed
wants to merge 9 commits into from
Closed

Conversation

amirylm
Copy link
Collaborator

@amirylm amirylm commented Sep 12, 2023

No description provided.

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@amirylm amirylm marked this pull request as ready for review September 13, 2023 12:41
@amirylm amirylm requested a review from a team as a code owner September 13, 2023 12:41
ferglor
ferglor previously approved these changes Sep 13, 2023
ctx, cancel := context.WithCancel(pctx)
defer cancel()

lggr := p.lggr.With("where", "reader")
lggr := p.lggr.Named(fmt.Sprintf("ReaderThread-%d", tid))
Copy link
Collaborator

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,
Copy link
Collaborator

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/registry.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)
Copy link
Contributor

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?

Copy link
Collaborator Author

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

@cl-sonarqube-production
Copy link

SonarQube Quality Gate

Quality Gate failed

Failed condition 71.9% 71.9% Coverage on New Code (is less than 80%)

See analysis details on SonarQube

Copy link
Contributor

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.

@github-actions github-actions bot added the Stale label Nov 20, 2023
@github-actions github-actions bot closed this Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants