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

Handle gRPC shutdown from context cancel #122

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

jacobweinstock
Copy link
Member

@jacobweinstock jacobweinstock commented Sep 1, 2022

Description

This fixes an issue where a single ctrl-c to hegel doesn't shutdown the gRPC server and requires another ctrl-c in order for hegel to exit at all. This causes deployments into envs like Kubernetes to have to be force-ably killed in order to shutdown.

Why is this needed

Fixes: #

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

This fixes an issue where a single ctrl-c to hegel doesn't
shutdown gRPC server and requires another ctrl-c in order
for hegel to exit at all. This causes deployments into envs
like Kubernetes to have to be force-ably killed in order to
shutdown.

Signed-off-by: Jacob Weinstock <[email protected]>
This allows the make target for building the image
to use whatever the host system has set for GOPROXY.

Signed-off-by: Jacob Weinstock <[email protected]>
@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #122 (672517f) into main (76ce4df) will decrease coverage by 0.10%.
The diff coverage is 0.00%.

❗ Current head 672517f differs from pull request most recent head de675ee. Consider uploading reports for the commit de675ee to get more accurate results

@@            Coverage Diff             @@
##             main     #122      +/-   ##
==========================================
- Coverage   25.26%   25.16%   -0.11%     
==========================================
  Files           9        9              
  Lines         930      934       +4     
==========================================
  Hits          235      235              
- Misses        659      663       +4     
  Partials       36       36              
Impacted Files Coverage Δ
grpc/server.go 42.59% <0.00%> (-1.08%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jacobweinstock jacobweinstock added the kind/bug Categorizes issue or PR as related to a bug. label Sep 1, 2022
@chrisdoherty4
Copy link
Member

@Mergifyio queue

@chrisdoherty4 chrisdoherty4 added the ready-to-merge Signal to Mergify to merge the PR. label Sep 2, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 2, 2022

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at beca565

@mergify mergify bot merged commit beca565 into tinkerbell:main Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants