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

Check that string probe port match container port #457

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jouve
Copy link
Contributor

@jouve jouve commented Nov 26, 2022

implementation of #455

if a probe port is a string and the string is not of number (like "1234" ) and the string is not in the container ports (ie. ` ports[].name), then raise a diagnostic message.

while the resource it accepted by kube currently, the liveness/readiness/startup will fail anyway, so added to default checks.

@jouve jouve requested a review from janisz as a code owner November 26, 2022 07:26
pkg/templates/probeport/template.go Outdated Show resolved Hide resolved
Parameters: params.ParamDescs,
ParseAndValidateParams: params.ParseAndValidate,
Instantiate: params.WrapInstantiateFunc(func(_ params.Params) (check.Func, error) {
return util.PerNonInitContainerCheck(func(container *v1.Container) []diagnostic.Diagnostic {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It will be great if you extract this to a named function and add unit tests to check all code paths.

@janisz janisz requested a review from dhaus67 November 28, 2022 12:31
pkg/templates/probeport/template.go Outdated Show resolved Hide resolved
e2etests/bats-tests.sh Outdated Show resolved Hide resolved
@janisz
Copy link
Collaborator

janisz commented Dec 6, 2022

@jouve You need to regenerate files

@jouve
Copy link
Contributor Author

jouve commented Dec 6, 2022

I'll update this PR soon :)

@janisz
Copy link
Collaborator

janisz commented Feb 22, 2023

@jouve ping

@janisz
Copy link
Collaborator

janisz commented Feb 28, 2024

@jouve Could you rebase? I think we can make it into next release
Don't forget to rerequest review ⟳

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.

3 participants