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

Create new check for liveness port #661

Merged

Conversation

charlesoconor
Copy link
Contributor

Want to ensure the port is correctly set for liveness probe since it's easy to make a typo when people are doing copy pasta. This will let you know it's failing airier and not have to wait for a failed deployment.

Want to do this also for startup and readiness checks. Assume it make sense to have them as other checks and was planning on doing that in a follow up.

Copy link
Collaborator

@janisz janisz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I've left some comments. I think we should use the same nomencalture as k8s so ports are exposed not opened. I think we should also support TCP probes not only http.

pkg/builtinchecks/yamls/liveness-port.yaml Outdated Show resolved Hide resolved
pkg/templates/livenessport/template.go Outdated Show resolved Hide resolved
pkg/templates/livenessport/template.go Outdated Show resolved Hide resolved
pkg/templates/livenessport/template.go Outdated Show resolved Hide resolved
pkg/templates/livenessport/template.go Show resolved Hide resolved
pkg/templates/livenessport/template.go Outdated Show resolved Hide resolved
pkg/templates/livenessport/template.go Outdated Show resolved Hide resolved
pkg/templates/livenessport/template.go Outdated Show resolved Hide resolved
tests/checks/invalid-target-ports.yaml Outdated Show resolved Hide resolved
@charlesoconor charlesoconor force-pushed the add-check-for-ports-in-liveness-prob branch from ec54b69 to 7df14b8 Compare November 27, 2023 20:19
@charlesoconor
Copy link
Contributor Author

Thanks for the review! Have updated with the requested changes.

@charlesoconor
Copy link
Contributor Author

@janisz, is there anything else you want me to change? Take another look when you have a chance.

Want to ensure the port is correctly set for liveness probe since it's
easy to make a typo when people are doing copy pasta. This will let you
know it's failing airier and not have to wait for a failed deployment.

Want to do this also for startup and readiness checks. Assume it make
sense to have them as other checks and was planning on doing that in a
follow up.
@charlesoconor charlesoconor force-pushed the add-check-for-ports-in-liveness-prob branch from 7df14b8 to 97b2cd9 Compare December 14, 2023 01:23
@janisz janisz self-requested a review December 21, 2023 17:00
@janisz
Copy link
Collaborator

janisz commented Dec 22, 2023

@charlesoconor Thank you. I'm sorry I missed your comment, you can re-request review to speed up the process as I'll see this PR on my https://github.com/pulls board

@janisz janisz merged commit d1ffb0a into stackrox:main Dec 22, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants