Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Wait on gRPC readiness (#10) #14

Merged
merged 3 commits into from
May 21, 2022
Merged

Wait on gRPC readiness (#10) #14

merged 3 commits into from
May 21, 2022

Conversation

ahdekkers
Copy link
Contributor

@ahdekkers ahdekkers commented May 6, 2022

Description

Added dependencies: context and grpc.

Fixes #10

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Running existing tests
  • Created new tests

Checklist:

  • My code has been linted (make lint)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes (make test lint build acceptance-test)
  • I have rebased my branch to include the latest changes from master

@ghost
Copy link

ghost commented May 6, 2022

👆 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@codecov
Copy link

codecov bot commented May 7, 2022

Codecov Report

Merging #14 (96bde53) into main (7dbfefd) will increase coverage by 1.31%.
The diff coverage is 100.00%.

❗ Current head 96bde53 differs from pull request most recent head 5ec0159. Consider uploading reports for the commit 5ec0159 to get more accurate results

@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
+ Coverage   84.40%   85.71%   +1.31%     
==========================================
  Files           2        2              
  Lines         109      119      +10     
==========================================
+ Hits           92      102      +10     
  Misses         15       15              
  Partials        2        2              
Impacted Files Coverage Δ
waitfor.go 79.26% <100.00%> (+2.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dbfefd...5ec0159. Read the comment docs.

@dnnrly
Copy link
Owner

dnnrly commented May 7, 2022

First of all, a massive thank you for this contribution! It really makes me happy to see people take this project seriously. I think what would make this even better are a couple of unit tests and an acceptance test to prove everything continues to work with future changes.

I'm happy to help you with that aspect if you feel you need it.

@ahdekkers
Copy link
Contributor Author

Hey! No worries, it's a neat repo :) Only just saw this but will get to writing those acceptance tests

@ahdekkers
Copy link
Contributor Author

ahdekkers commented May 14, 2022

Let me know what you think @dnnrly. Added a dependency on freeport in order to setup the grpc server with any free port.

@dnnrly
Copy link
Owner

dnnrly commented May 15, 2022

I have absolutely no problem with that dependency! Will hopefully get a bit of time to look at this in next couple of days.

@dnnrly
Copy link
Owner

dnnrly commented May 19, 2022

Sorry for taking so long to around to this. The code looks good. Any chance that you'd be able to extend the documentation to include the new grpc option?

@ahdekkers
Copy link
Contributor Author

Sure, will get to it in the near future

@ahdekkers
Copy link
Contributor Author

Made the documentation, let me know what you think @dnnrly

@dnnrly
Copy link
Owner

dnnrly commented May 21, 2022

Excellent! Thank you so much for your contribution. If you do twitter and you're prepared to share your handle, you just earned another follower. 😄

@dnnrly dnnrly merged commit 962ea2d into dnnrly:main May 21, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wait on gRPC readiness
2 participants