-
Notifications
You must be signed in to change notification settings - Fork 452
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 exposed svc ports #778
Check exposed svc ports #778
Conversation
Signed-off-by: Yuri Sa <[email protected]>
Signed-off-by: Yuri Sa <[email protected]>
Resolves #257 |
Signed-off-by: Yuri Sa <[email protected]>
Signed-off-by: Yuri Sa <[email protected]>
Signed-off-by: Yuri Sa <[email protected]>
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
Signed-off-by: Yuri Sa <[email protected]>
@yuriolisa CI failed |
Signed-off-by: Yuri Sa <[email protected]>
Signed-off-by: Yuri Sa <[email protected]>
@pavolloffay, CI is fixed. |
@yuriolisa IMO The changes in this PR are a bit tricky in operator we should not care much about If the others feel this adds value and make sense in the operator end. I would definitely recommend @yuriolisa to add e2e tests with different config types to completely validate the corner cases. |
Correct, we should not fail or return errors when this happens, we should just ignore and continue. In this case, if there are no pipelines, there should be no ports being exposed. |
Signed-off-by: Yuri Sa <[email protected]>
@jpkrohling , @VineethReddy02 , @pavolloffay . Could you please review it? |
Signed-off-by: Yuri Sa <[email protected]>
Signed-off-by: Yuri Sa <[email protected]>
Signed-off-by: Yuri Sa <[email protected]>
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, but would be good to have a review from a fresh pair of eyes. @pavolloffay, @VineethReddy02?
@VineethReddy02, given the outcome of your previous review, would you also be able to take a look at this? |
* Creating check if services are configured properly Signed-off-by: Yuri Sa <[email protected]> * Creating check if services are configured properly Signed-off-by: Yuri Sa <[email protected]>
Signed-off-by: Yuri Sa [email protected]
[x] Created function
ConfigValidate
which checks if the services are present in the pipeline set.[x] Created unit test to validate the new function.
[x] Included validation on
ConfigToReceiverPorts
function to open only ports for enabled receivers.[x] Changed
config_to_ports_test.go
:- Fixing indentation.
- Included service spec in tested config string.
Resolves #257