-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Allow probes to explicitly set the port to the containerPort (#8288) #12225
Conversation
Codecov Report
@@ Coverage Diff @@
## main #12225 +/- ##
==========================================
- Coverage 87.36% 87.32% -0.05%
==========================================
Files 196 195 -1
Lines 9611 9583 -28
==========================================
- Hits 8397 8368 -29
- Misses 935 936 +1
Partials 279 279
Continue to review full report at Codecov.
|
pkg/apis/serving/k8s_validation.go
Outdated
func validateContainersPorts(containers []corev1.Container) *apis.FieldError { | ||
// validateContainersPorts validates port when specified multiple containers, | ||
// and returns the single serving port if error is nil | ||
func validateContainersPorts(containers []corev1.Container) (*apis.FieldError, intstr.IntOrString) { |
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.
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.
Thanks. I'll admit that I sort of just got annoyed and skipped a bunch of other stuff I was doing yesterday when I realized that I'd bumped this bug but hadn't actually made sure it got fixed.
pkg/apis/serving/k8s_validation.go
Outdated
var count int | ||
var port = intstr.IntOrString{IntVal: 8080, StrVal: "http"} |
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.
fwiw I got a bit confused about this use of IntOrString because we're really using it as an IntAndString here I think (which is a bit at odds with the Type field in IntOrString and some of its methods). What do you think about returning the matched ContainerPort struct instead (and then we could use the explicit Name and ContainerPort fields in validateProbe)?
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.
/shrug
Sure, why not.
Ready for another look |
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: evankanderson, julz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cherry-pick release-1.0 |
@dprotaso: new pull request created: #12270 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Fixes #8288
Proposed Changes
Release Note