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

Separated RpcServer & RestServer from aggregator #204

Merged
merged 5 commits into from
Jun 17, 2024
Merged

Separated RpcServer & RestServer from aggregator #204

merged 5 commits into from
Jun 17, 2024

Conversation

taco-paco
Copy link
Contributor

This closes: Specify Aggregator methods to be exposed in RPC. Done as result of below
And tech debt of separating rpc and rest servers from aggregator

@taco-paco taco-paco requested review from Hyodar and emlautarom1 June 3, 2024 12:06
Hyodar
Hyodar approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@Hyodar Hyodar left a comment

Choose a reason for hiding this comment

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

GJ, LGTM. The only consideration I have is in terms of separately declaring the RPC and REST clients, but let's keep the discussion of that in the comment.

aggregator/cmd/main.go Show resolved Hide resolved
aggregator/cmd/main.go Outdated Show resolved Hide resolved
@Hyodar Hyodar requested a review from jmederosalvarado June 5, 2024 13:17
@taco-paco taco-paco mentioned this pull request Jun 6, 2024
Merged
emlautarom1
emlautarom1 suggested changes Jun 7, 2024
View reviewed changes
aggregator/aggregator.go Show resolved Hide resolved
aggregator/rest_server/server.go Show resolved Hide resolved
aggregator/rest_server/server.go Show resolved Hide resolved
aggregator/rpc_server/server.go Show resolved Hide resolved
emlautarom1
emlautarom1 suggested changes Jun 7, 2024
View reviewed changes
Copy link
Contributor

@emlautarom1 emlautarom1 left a comment

Choose a reason for hiding this comment

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

Some small details that could have been overlooked but overall LGTM

@taco-paco taco-paco force-pushed the refactor/aggregator branch from 0196995 to c707f79 Compare June 17, 2024 10:34
@taco-paco taco-paco force-pushed the refactor/aggregator branch from c707f79 to b75808f Compare June 17, 2024 10:54
emlautarom1
emlautarom1 approved these changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@emlautarom1 emlautarom1 left a comment

Choose a reason for hiding this comment

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

LGTM

@taco-paco taco-paco merged commit 50e6705 into main Jun 17, 2024
5 checks passed
@Hyodar Hyodar deleted the refactor/aggregator branch October 3, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hyodar Hyodar Hyodar approved these changes

@emlautarom1 emlautarom1 emlautarom1 approved these changes

@jmederosalvarado jmederosalvarado Awaiting requested review from jmederosalvarado

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
@taco-paco @Hyodar @emlautarom1