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

merge Engine with Server #383

Merged
merged 3 commits into from
Dec 24, 2023

Conversation

snowAvocado
Copy link
Contributor

Ticket(s)

Description

Related PRs

Development Checklist

  • I have added a descriptive title to this PR.
  • I have squashed related commits together.
  • I have rebased my branch on top of the latest main branch.
  • I have performed a self-review of my own code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added docstring(s) and type annotations to my code.
  • I have made corresponding changes to the documentation (docs).
  • I have added tests for my changes.

Legal Checklist

@snowAvocado snowAvocado changed the title merge engine to server merge Engine with Server Dec 22, 2023
@snowAvocado
Copy link
Contributor Author

#347

@mostafa mostafa linked an issue Dec 22, 2023 that may be closed by this pull request
Copy link
Member

@mostafa mostafa left a comment

Choose a reason for hiding this comment

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

Hey @snowAvocado,

Thanks for your contribution! 🙏 I have two comments:

  1. Please fix linter errors (and ignore unimportant ones).
  2. You can merge the stop with shutdown and unexport the stop method, effectively removing it from the interface.

network/server.go Outdated Show resolved Hide resolved
mostafa
mostafa previously approved these changes Dec 23, 2023
@mostafa mostafa dismissed their stale review December 23, 2023 12:14

Please sign your commits.

@mostafa
Copy link
Member

mostafa commented Dec 23, 2023

@snowAvocado Please sign your commits.

@snowAvocado snowAvocado force-pushed the merge-engine-to-server branch from 8002e43 to eb44f41 Compare December 24, 2023 07:05
@snowAvocado snowAvocado force-pushed the merge-engine-to-server branch from 3f81aa1 to 214afaf Compare December 24, 2023 07:26
Copy link
Member

@mostafa mostafa left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@mostafa mostafa merged commit 890d562 into gatewayd-io:main Dec 24, 2023
3 of 4 checks passed
@snowAvocado snowAvocado deleted the merge-engine-to-server branch December 24, 2023 15:30
@snowAvocado snowAvocado restored the merge-engine-to-server branch December 24, 2023 15:36
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.

Merge Engine with Server
2 participants