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

GRPC healthcheck should not have keepalive time #5899

Merged
merged 10 commits into from
Jul 2, 2024
Merged

GRPC healthcheck should not have keepalive time #5899

merged 10 commits into from
Jul 2, 2024

Conversation

j1m-ryan
Copy link
Contributor

@j1m-ryan j1m-ryan commented Jul 1, 2024 •
edited
Loading

Proposed changes

#4996

  • Ensure keepalive is not added to grpc health check
  • Improve formatting of grpc health check directive
  • Remove flaky label from grpc related tests (as they are running locally fine, open to adding this back however)
  • Add test for grpc health check by simulating a health check fail, where the status does not match the expected status, and expecting a line in the nginx log
  • Update snapshots

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@j1m-ryan j1m-ryan requested a review from a team as a code owner July 1, 2024 10:10
@github-actions github-actions bot added bug An issue reporting a potential bug go Pull requests that update Go code python Pull requests that update Python code tests Pull requests that update tests labels Jul 1, 2024
@j1m-ryan j1m-ryan changed the title Fix/grpc no keepalive time Fix: GRPC healthcheck should not have keepalive time Jul 1, 2024
@j1m-ryan j1m-ryan added the needs cherry pick Cherry pick this PR into a release branch label Jul 1, 2024
@j1m-ryan j1m-ryan merged commit f154d30 into main Jul 2, 2024
84 checks passed
@j1m-ryan j1m-ryan deleted the fix/grpc-no-keepalive-time branch July 2, 2024 13:57
nginx-bot pushed a commit that referenced this pull request Jul 2, 2024
@j1m-ryan @web-flow
Fix: GRPC healthcheck should not have keepalive time (#5899)
60c3975 
* fix template file, and fix test

* add test for GPRC health check fail message, remove flaky label from prior tests, clean up health check template

* update snap

* update snapshots

* remove newline

* fix tests
j1m-ryan added a commit that referenced this pull request Jul 2, 2024
@nginx-bot @j1m-ryan
[cherry-pick] Fix: GRPC healthcheck should not have keepalive time (#…
459d4d3 
…5921)

Fix: GRPC healthcheck should not have keepalive time (#5899)

* fix template file, and fix test

* add test for GPRC health check fail message, remove flaky label from prior tests, clean up health check template

* update snap

* update snapshots

* remove newline

* fix tests

Co-authored-by: Jim Ryan <[email protected]>
@pdabelf5 pdabelf5 changed the title Fix: GRPC healthcheck should not have keepalive time GRPC healthcheck should not have keepalive time Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaun-nx shaun-nx shaun-nx approved these changes

@jjngx jjngx jjngx approved these changes

@AlexFenlon AlexFenlon AlexFenlon approved these changes

@oseoin oseoin oseoin approved these changes

Assignees
No one assigned
Labels
bug An issue reporting a potential bug go Pull requests that update Go code needs cherry pick Cherry pick this PR into a release branch python Pull requests that update Python code tests Pull requests that update tests
Projects
NGINX Ingress Controller
Status: Done 🚀
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@j1m-ryan @oseoin @AlexFenlon @jjngx @shaun-nx