Skip to content

core/services/chainlink/config.go: merge RawConfigs using expected fields - #14433

Merged
archseer merged 14 commits into
developfrom
merge-raw-config
Nov 7, 2024
Merged

core/services/chainlink/config.go: merge RawConfigs using expected fields#14433
archseer merged 14 commits into
developfrom
merge-raw-config

Conversation

@cfal

@cfal cfal commented Sep 14, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@cfal
cfal requested review from archseer and jmank88 September 14, 2024 21:42
@cl-sonarqube-production

Copy link
Copy Markdown

Quality Gate failed Quality Gate failed

Failed conditions
7 New Major Issues (required ≤ 5)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

@cfal
cfal force-pushed the merge-raw-config branch 3 times, most recently from 0048f6b to 2e87568 Compare October 17, 2024 21:17
@github-actions

github-actions Bot commented Oct 17, 2024

Copy link
Copy Markdown
Contributor

AER Report: CI Core

aer_workflow , commit , Detect Changes , Clean Go Tidy & Generate , lint , Core Tests (go_core_tests) , Core Tests (go_core_ccip_deployment_tests) , Core Tests (go_core_race_tests) , Core Tests (go_core_fuzz) , SonarQube Scan

1. Error return value not checked: [Golang Lint]

Source of Error:
core/services/chainlink/config.go:66:17: Error return value of `(github.com/smartcontractkit/chainlink/v2/core/services/chainlink.RawConfig).SetFrom` is not checked (errcheck)
			rs[i].SetFrom(config)
			 ^
**Why**: The function `SetFrom` returns an error that is not being checked. Ignoring error return values can lead to unhandled exceptions and unexpected behavior.

Suggested fix: Add error handling for the SetFrom function call to ensure any errors are properly managed.

2. Variable naming issues: [Golang Lint]

Source of Error:
core/services/chainlink/config.go:60:3: var-naming: var chainId should be chainID (revive)
		chainId := config.ChainID()
		^

core/services/chainlink/config.go:62:4: var-naming: var otherChainId should be otherChainID (revive)
			otherChainId := r.ChainID()
			^

core/services/chainlink/config.go:82:2: var-naming: var chainIds should be chainIDs (revive)
	chainIds := commonconfig.UniqueStrings{}
	^

core/services/chainlink/config.go:84:3: var-naming: var chainId should be chainID (revive)
		chainId := config.ChainID()
		^

core/services/chainlink/config.go:112:2: var-naming: var chainId should be chainID (revive)
	chainId := c.ChainID()
	^
**Why**: The variable names do not follow the Go naming conventions, which prefer `ID` over `Id` for consistency and readability.

Suggested fix: Rename the variables from chainId, otherChainId, and chainIds to chainID, otherChainID, and chainIDs respectively.

3. Redundant nil check: [Golang Lint]

Source of Error:
core/services/chainlink/config.go:117:5: S1009: should omit nil check; len() for []string is defined as zero (gosimple)
	if nodeNames == nil || len(nodeNames) == 0 {
	 ^
**Why**: Checking for `nil` before checking the length of a slice is redundant because `len()` on a `nil` slice returns zero.

Suggested fix: Remove the nil check and only check the length of the slice: if len(nodeNames) == 0 {.

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@cfal
cfal force-pushed the merge-raw-config branch from 2e87568 to f3d4531 Compare October 21, 2024 09:25
@github-actions

github-actions Bot commented Oct 21, 2024

Copy link
Copy Markdown
Contributor

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@cfal
cfal force-pushed the merge-raw-config branch from f3d4531 to 1feb9eb Compare October 21, 2024 09:30
@cfal
cfal force-pushed the merge-raw-config branch from 1feb9eb to 2c14de1 Compare October 28, 2024 17:22
@cfal
cfal marked this pull request as ready for review October 28, 2024 17:24
@cfal
cfal requested review from a team as code owners October 28, 2024 17:24
archseer
archseer previously approved these changes Oct 29, 2024
@bolekk

bolekk commented Oct 29, 2024

Copy link
Copy Markdown
Contributor

@cfal can you clean up this PR and get it merged please? Aptos staging got affected by some config reorgs and I believe this will fix it.

@cfal
cfal force-pushed the merge-raw-config branch from 2c14de1 to 8d14950 Compare October 30, 2024 08:43
@cfal
cfal requested a review from a team as a code owner October 30, 2024 08:43
@cfal
cfal force-pushed the merge-raw-config branch from 8d14950 to e9bf7df Compare October 30, 2024 08:56
@cfal
cfal requested review from archseer and bolekk October 30, 2024 09:17
Comment thread core/services/chainlink/config_test.go Outdated
Comment thread core/services/chainlink/config.go Outdated
Comment thread core/services/chainlink/config.go Outdated
@cfal
cfal force-pushed the merge-raw-config branch from 7acf14c to db1952a Compare October 30, 2024 15:34
@cfal
cfal requested a review from jmank88 October 30, 2024 15:35
@cfal
cfal force-pushed the merge-raw-config branch 3 times, most recently from 79a356a to c7ed17d Compare October 30, 2024 15:48
@cfal
cfal force-pushed the merge-raw-config branch from 69b8628 to 8a95670 Compare November 6, 2024 20:15
@cfal
cfal requested a review from a team as a code owner November 6, 2024 20:15
@cfal
cfal requested a review from jmank88 November 6, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants