Add an interface to replace the ChangeSet function type. #16186

cgruber · 2025-02-01T02:02:45Z

This preserves the existing function type (as an alias to a new function type, ChangeLogic). It adds also two function types ChangeLogic and PreconditionVerifier, to encapsulate the logic of these two functions. The interface exposes an Apply and a VerifyPreconditions method. The aforementioned function types can be used with a convenience function to generate an implementation of ChangeSetV2. Alternatively, a full implementation of the interface can be built using a struct that implements those methods.

DPA-1527

github-actions · 2025-02-01T02:38:49Z

AER Report: CI Core

aer_workflow , commit , Detect Changes , Clean Go Tidy & Generate , Scheduled Run Frequency , GolangCI Lint (integration-tests) , GolangCI Lint (deployment) , Core Tests (go_core_tests) , Core Tests (go_core_tests_integration) , Core Tests (go_core_ccip_deployment_tests) , test-scripts , Core Tests (go_core_fuzz) , Core Tests (go_core_race_tests) , lint , SonarQube Scan

1. Ineffectual assignment to err: [Golang Lint (deployment)]

Source of Error:

Golang Lint (deployment) 2025-02-04T04:41:02.9104204Z ##[error]deployment/environment/crib/ccip_deployer.go:58:6: ineffectual assignment to err (ineffassign)
Golang Lint (deployment) 2025-02-04T04:41:02.9104967Z 	*e, err = commonchangeset.ApplyChangesets(nil, *e, nil, []commonchangeset.ConfiguredChangeSet{
Golang Lint (deployment) 2025-02-04T04:41:02.9105541Z 	 ^
Golang Lint (deployment) 2025-02-04T04:41:02.9106328Z ##[error]deployment/environment/crib/ccip_deployer.go:132:6: ineffectual assignment to err (ineffassign)
Golang Lint (deployment) 2025-02-04T04:41:02.9107156Z 	*e, err = commonchangeset.ApplyChangesets(nil, *e, nil, []commonchangeset.ConfiguredChangeSet{
Golang Lint (deployment) 2025-02-04T04:41:02.9107644Z 	 ^

Why: The variable err is assigned a value but never used, which is flagged by the ineffassign linter.

Suggested fix: Ensure that the err variable is checked or used after assignment to handle any potential errors.

2. Use assert.NoError instead of assert.Equal: [Golang Lint (deployment)]

Source of Error:

Golang Lint (deployment) 2025-02-04T04:41:02.9108354Z ##[error]deployment/changeset_test.go:28:2: error-nil: use assert.NoError (testifylint)
Golang Lint (deployment) 2025-02-04T04:41:02.9108847Z 	assert.Equal(t, nil, verify)
Golang Lint (deployment) 2025-02-04T04:41:02.9109147Z 	^
Golang Lint (deployment) 2025-02-04T04:41:02.9110048Z ##[error]deployment/changeset_test.go:47:2: error-nil: use assert.NoError (testifylint)
Golang Lint (deployment) 2025-02-04T04:41:02.9110633Z 	assert.Equal(t, nil, verify)
Golang Lint (deployment) 2025-02-04T04:41:02.9110927Z 	^
Golang Lint (deployment) 2025-02-04T04:41:02.9111630Z ##[error]deployment/changeset_test.go:67:2: error-nil: use assert.NoError (testifylint)
Golang Lint (deployment) 2025-02-04T04:41:02.9112151Z 	assert.Equal(t, nil, verify)
Golang Lint (deployment) 2025-02-04T04:41:02.9112430Z 	^

Why: The testify linter suggests using assert.NoError instead of assert.Equal for checking if an error is nil, which is more idiomatic and clear.

Suggested fix: Replace assert.Equal(t, nil, verify) with assert.NoError(t, verify).

3. Use require.NoError instead of assert.NoError: [Golang Lint (deployment)]

Source of Error:

Golang Lint (deployment) 2025-02-04T04:41:02.9113268Z ##[error]deployment/changeset_test.go:79:2: require-error: for error assertions use require (testifylint)
Golang Lint (deployment) 2025-02-04T04:41:02.9113794Z 	assert.NoError(t, verify)
Golang Lint (deployment) 2025-02-04T04:41:02.9114134Z 	^

Why: The testify linter suggests using require.NoError for error assertions to ensure the test fails immediately if the error is not nil.

Suggested fix: Replace assert.NoError(t, verify) with require.NoError(t, verify).

4. Function name stutters: [Golang Lint (deployment)]

Source of Error:

Golang Lint (deployment) 2025-02-04T04:41:02.9101032Z ##[error]deployment/common/changeset/test_helpers.go:20:6: exported: func name will be used as changeset.ChangesetApplication by other packages, and that stutters; consider calling this Application (revive)
Golang Lint (deployment) 2025-02-04T04:41:02.9102185Z func ChangesetApplication[C any](
Golang Lint (deployment) 2025-02-04T04:41:02.9102558Z ^

Why: The function name ChangesetApplication stutters when used with the package name, which is flagged by the revive linter.

Suggested fix: Rename the function to Application to avoid stuttering when used with the package name.

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

github-actions · 2025-02-04T02:02:32Z

Flakeguard Summary

Ran new or updated tests between develop and c9299c5 (DPA-1527-Rework-ChangeSet-to-be-an-interface-likely-in-multiple-steps).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

Name	Pass Ratio	Panicked?	Timed Out?	Race?	Runs	Successes	Failures	Skips	Package	Package Panicked?	Avg Duration	Code Owners
TestSmokeState	0.00%	true	true	false	3	0	3	0	github.com/smartcontractkit/chainlink/deployment/ccip/changeset	true	0s	@AnieeG, @kylesmartin, @smartcontractkit/ccip-offchain, @smartcontractkit/deployment-automation

Artifacts

For detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json.

github-actions · 2025-02-04T03:38:25Z

Flakeguard Summary

Ran new or updated tests between develop and 6b77dc2 (DPA-1527-Rework-ChangeSet-to-be-an-interface-likely-in-multiple-steps).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

Name	Pass Ratio	Panicked?	Timed Out?	Race?	Runs	Successes	Failures	Skips	Package	Package Panicked?	Avg Duration	Code Owners
TestSmokeState	0.00%	true	true	false	2	0	2	0	github.com/smartcontractkit/chainlink/deployment/ccip/changeset	true	0s	@AnieeG, @kylesmartin, @smartcontractkit/ccip-offchain, @smartcontractkit/deployment-automation

Artifacts

For detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json.

This preserves the existing function type (as an alias to a new function type, ChangeLogic). It adds also two function types ChangeLogic and PreconditionVerifier, to encapsulate the logic of these two functions. The interface exposes an Apply and a VerifyPreconditions method. The aforementioned function types can be used with a convenience function to generate an implementation of ChangeSetV2. Alternatively, a full implementation of the interface can be built using a struct that implements those methods.

…ngeSet, which can be generic, and therefore avoid a lot of mess in dealing with the WrapChangeset stuff.

…re now handled by the ConfiguredChangeSet interface.

…ructure, and add some tests to make sure that works.

cl-sonarqube-production · 2025-02-04T04:59:18Z

Quality Gate failed

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

See analysis details on SonarQube

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

github-actions · 2025-02-04T05:24:59Z

Flakeguard Summary

Ran new or updated tests between develop and b530654 (DPA-1527-Rework-ChangeSet-to-be-an-interface-likely-in-multiple-steps).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

Name	Pass Ratio	Panicked?	Timed Out?	Race?	Runs	Successes	Failures	Skips	Package	Package Panicked?	Avg Duration	Code Owners
TestEmptyBatch	66.67%	true	true	false	3	2	1	0	github.com/smartcontractkit/chainlink/deployment/ccip/changeset	true	8.18s	@AnieeG, @kylesmartin, @smartcontractkit/ccip-offchain, @smartcontractkit/deployment-automation
TestSmokeState	50.00%	true	true	false	2	1	1	0	github.com/smartcontractkit/chainlink/deployment/ccip/changeset	true	11.5s	@AnieeG, @kylesmartin, @smartcontractkit/ccip-offchain, @smartcontractkit/deployment-automation

Artifacts

For detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json.

RodrigoAD · 2025-02-04T09:58:45Z

deployment/changeset.go

+}
+
+func (scs simpleChangeSet[C]) Apply(e Environment, config C) (ChangesetOutput, error) {
+	return scs.apply(e, config)


Do we expect users to always validate the config before applying the changeset? If so, we could enforce running c.VerifyPreconditions here, so users do not need to explicitly call Verify before Apply

I don't want to enforce it here, because this is a simple changeset composed of two pure functions, and not everyone may implement using this. This is purely a wrapper around two functions.

The execution environment that calls cs.Apply(...) should call verify before apply, and I do that in this PR, in the test_helpers in the ConfiguredChangeSet wrapper, which is what the execution environment uses. Similarly, we'd do that in the migrations side.

Or to phrase it otherwise, users will never call this method. The infrastructure will, and does.

cgruber force-pushed the DPA-1527-Rework-ChangeSet-to-be-an-interface-likely-in-multiple-steps branch from 14b0e8a to 15ed30f Compare February 1, 2025 02:16

cgruber marked this pull request as ready for review February 4, 2025 02:54

cgruber requested review from a team, AnieeG and kylesmartin as code owners February 4, 2025 02:54

cgruber requested review from winder, asoliman92, makramkd, dimkouv, mateusz-sekara, 0xAustinWang and 0xnogo February 4, 2025 02:54

cgruber added 5 commits February 3, 2025 20:35

Rework ChangesetApplication to pass around an interface ConfiguredCha…

01d7a70

…ngeSet, which can be generic, and therefore avoid a lot of mess in dealing with the WrapChangeset stuff.

Pull out WrapChangeset, which existed to handle some generics which a…

e358937

…re now handled by the ConfiguredChangeSet interface.

Wire in validation to the execution of changesets in the test infrast…

8ee7b56

…ructure, and add some tests to make sure that works.

Fix a missing caller from upstream.

b530654

cgruber force-pushed the DPA-1527-Rework-ChangeSet-to-be-an-interface-likely-in-multiple-steps branch from 6b77dc2 to b530654 Compare February 4, 2025 04:37

RodrigoAD reviewed Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an interface to replace the ChangeSet function type. #16186

Add an interface to replace the ChangeSet function type. #16186

cgruber commented Feb 1, 2025 •

edited by jira bot

Loading

github-actions bot commented Feb 1, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025

github-actions bot commented Feb 4, 2025

cl-sonarqube-production bot commented Feb 4, 2025

github-actions bot commented Feb 4, 2025

RodrigoAD Feb 4, 2025

cgruber Feb 5, 2025

cgruber Feb 5, 2025

Add an interface to replace the ChangeSet function type. #16186

Are you sure you want to change the base?

Add an interface to replace the ChangeSet function type. #16186

Conversation

cgruber commented Feb 1, 2025 • edited by jira bot Loading

github-actions bot commented Feb 1, 2025 • edited Loading

AER Report: CI Core

1. Ineffectual assignment to err: [Golang Lint (deployment)]

2. Use assert.NoError instead of assert.Equal: [Golang Lint (deployment)]

3. Use require.NoError instead of assert.NoError: [Golang Lint (deployment)]

4. Function name stutters: [Golang Lint (deployment)]

AER Report: Operator UI CI ran successfully ✅

github-actions bot commented Feb 4, 2025

Flakeguard Summary

Found Flaky Tests ❌

Artifacts

github-actions bot commented Feb 4, 2025

Flakeguard Summary

Found Flaky Tests ❌

Artifacts

cl-sonarqube-production bot commented Feb 4, 2025

Quality Gate failed

github-actions bot commented Feb 4, 2025

Flakeguard Summary

Found Flaky Tests ❌

Artifacts

RodrigoAD Feb 4, 2025

Choose a reason for hiding this comment

cgruber Feb 5, 2025

Choose a reason for hiding this comment

cgruber Feb 5, 2025

Choose a reason for hiding this comment

cgruber commented Feb 1, 2025 •

edited by jira bot

Loading

github-actions bot commented Feb 1, 2025 •

edited

Loading