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

feat: add data stream clients #16437

Merged
merged 5 commits into from
Feb 19, 2025
Merged

feat: add data stream clients #16437

merged 5 commits into from
Feb 19, 2025

Conversation

ChrisAmora
Copy link
Contributor

@ChrisAmora ChrisAmora commented Feb 17, 2025 •
edited by jira bot
Loading

Description

Adds clients needed for data streams deploy.

References

@github-actions GitHub Actions
Copy link
Contributor

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

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@ChrisAmora ChrisAmora marked this pull request as ready for review February 18, 2025 11:12
@ChrisAmora ChrisAmora requested review from a team as code owners February 18, 2025 11:12
giogam
giogam reviewed Feb 18, 2025
View reviewed changes
Copy link
Collaborator

@giogam giogam left a comment

Choose a reason for hiding this comment

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

Overall looks good! Left a few comments

@ChrisAmora ChrisAmora force-pushed the feat/data-stream-clients branch from 2a8bffb to bf8f4fb Compare February 18, 2025 18:34
@github-actions GitHub Actions
Copy link
Contributor

Flakeguard Summary

Ran new or updated tests between develop and bf8f4fb (feat/data-stream-clients).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

2 Results
Name Pass Ratio Panicked? Timed Out? Race? Runs Successes Failures Skips Package Package Panicked? Avg Duration Code Owners
TestRegisterNodesInput_Validate 0% false false false 3 0 3 0 github.com/smartcontractkit/chainlink/deployment/data-streams/changeset false 0s @smartcontractkit/ccip-tooling, @smartcontractkit/ccip-offchain, @smartcontractkit/keystone, @smartcontractkit/core, @smartcontractkit/deployment-automation
TestRegisterNodesInput_Validate/missing_BootstrapNode 0% false false false 3 0 3 0 github.com/smartcontractkit/chainlink/deployment/data-streams/changeset false 0s @smartcontractkit/ccip-tooling, @smartcontractkit/ccip-offchain, @smartcontractkit/keystone, @smartcontractkit/core, @smartcontractkit/deployment-automation

Artifacts

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

@github-actions GitHub Actions
Copy link
Contributor

Flakeguard Summary

Ran new or updated tests between develop and bf8f4fb (feat/data-stream-clients).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

2 Results
Name Pass Ratio Panicked? Timed Out? Race? Runs Successes Failures Skips Package Package Panicked? Avg Duration Code Owners
TestRegisterNodesInput_Validate 0% false false false 6 0 6 0 github.com/smartcontractkit/chainlink/deployment/data-streams/changeset false 0s @smartcontractkit/ccip-tooling, @smartcontractkit/ccip-offchain, @smartcontractkit/keystone, @smartcontractkit/core, @smartcontractkit/deployment-automation
TestRegisterNodesInput_Validate/missing_BootstrapNode 0% false false false 6 0 6 0 github.com/smartcontractkit/chainlink/deployment/data-streams/changeset false 0s @smartcontractkit/ccip-tooling, @smartcontractkit/ccip-offchain, @smartcontractkit/keystone, @smartcontractkit/core, @smartcontractkit/deployment-automation

Artifacts

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

@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

@ChrisAmora ChrisAmora enabled auto-merge February 18, 2025 21:44
giogam
giogam reviewed Feb 19, 2025
View reviewed changes
deployment/data-streams/clients/entity_tool.go

func NewGetOverridesRequest(asset, quote string, product ...string) *GetOverridesRequest {
var prod *string
if len(product) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The variadic product parameter is there to avoid explicitly passing nil when calling NewGetOverridesRequest without a product, right? Nice!

giogam
giogam approved these changes Feb 19, 2025
View reviewed changes
Copy link
Collaborator

@giogam giogam left a comment

Choose a reason for hiding this comment

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

LGTM

@giogam
Copy link
Collaborator

giogam commented Feb 19, 2025

@ChrisAmora Not a blocker for this PR, but something to keep in mind—I wonder if there are any additional requirements for successfully making HTTP calls to the external adapters within the GitHub CI

@ChrisAmora
Copy link
Contributor Author

@ChrisAmora Not a blocker for this PR, but something to keep in mind—I wonder if there are any additional requirements for successfully making HTTP calls to the external adapters within the GitHub CI

I don't think so but we can check with Ivaylo when he comes back.

ecPablo
ecPablo approved these changes Feb 19, 2025
View reviewed changes
Copy link
Contributor

@ecPablo ecPablo left a comment

Choose a reason for hiding this comment

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

Left some minor comments / questions but otherwise lgtm!

deployment/data-streams/changeset/jd_register_nodes.go
Labels: labels,
})
if err != nil {
e.Logger.Errorw("failed to register node", "node", node.Name, "error", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we return the error here? or is it intentional to continue if not registered succesfully?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is Intentional, but I left a comment so we can revisit this if necessary in the future.

deployment/data-streams/clients/external_adapter.go
Pass: false,
StatusCode: resp.StatusCode,
ErrorMessage: resp.Status,
Data: errData,
Copy link
Contributor

Choose a reason for hiding this comment

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

Also return error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am following the same structure as https://github.com/smartcontractkit/gauntlet-plus-plus/blob/main/packages-ethereum/operations-mercury/src/providers/external-adapter/api-client.ts#L8
We may revisit this in future though.

@ChrisAmora ChrisAmora added this pull request to the merge queue Feb 19, 2025
Merged via the queue into develop with commit 95fa633 Feb 19, 2025
163 checks passed
@ChrisAmora ChrisAmora deleted the feat/data-stream-clients branch February 19, 2025 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giogam giogam giogam approved these changes

@ecPablo ecPablo ecPablo approved these changes

@winder winder Awaiting requested review from winder winder is a code owner automatically assigned from smartcontractkit/ccip-offchain

@asoliman92 asoliman92 Awaiting requested review from asoliman92 asoliman92 is a code owner automatically assigned from smartcontractkit/ccip-offchain

@makramkd makramkd Awaiting requested review from makramkd makramkd is a code owner automatically assigned from smartcontractkit/ccip-offchain

@dimkouv dimkouv Awaiting requested review from dimkouv dimkouv is a code owner automatically assigned from smartcontractkit/ccip-offchain

@mateusz-sekara mateusz-sekara Awaiting requested review from mateusz-sekara mateusz-sekara is a code owner automatically assigned from smartcontractkit/ccip-offchain

@0xAustinWang 0xAustinWang Awaiting requested review from 0xAustinWang 0xAustinWang is a code owner automatically assigned from smartcontractkit/ccip-offchain

@0xnogo 0xnogo Awaiting requested review from 0xnogo 0xnogo is a code owner automatically assigned from smartcontractkit/ccip-offchain

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ChrisAmora @giogam @ecPablo