This repository has been archived by the owner on Nov 20, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
adding health checks #432
adding health checks #432
Changes from 30 commits
7e6c2ef
8b3e89c
57dc3f4
616df58
3912db6
24b0459
d4fbdeb
e95a361
56b9869
9dcc606
3f48f54
3b058b3
4659b51
ee9b374
8f396c7
0276a09
569df2a
f22149c
f452feb
aa52cd0
22e506e
7a72a3e
440cf1b
e86cda1
b41ccff
1d3d03e
f89e630
726e2a4
7a29291
65c7ae0
ffef174
6280485
a116b4b
eb13a21
602a2bc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't need
!
here, other similar cases in this file as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. I removed it and it didn't throw any warnings. probably because it's inside a block that runs if
builderHttp.Protocol != null
.It's weird because at times the compiler will throw a warning when not using the
!
operator, even though the expression is in a context where it's guaranteed to not be null...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found an example: if you go to
ReplicaMonitor.cs
:113 and remove the!
afterserviceDesc.Readiness
, the compiler will throw a warning, even though there is no way for that variable to be null at that point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does feel a bit awkward to have health checks per service rather than per endpoint, but it seems to be what k8s does. Or maybe we require a port?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requiring a port will indeed make it more compatible with k8s, but it will make the
tye.yaml
more verbose and less maintainable in my opinion. one thing that k8s does to help with maintainability is to allow to specify a variable as a port. e.g.