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

Handle None being returned for ingresses #353

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

motjuste
Copy link

@motjuste motjuste commented Nov 24, 2023

svc.status.loadBalancer.ingress has been observed to return None in certain situations and leads to failure of the ingress-relation-created hook. This change handles the None and treats the situation as if no ingresses are present.

@motjuste motjuste requested a review from a team as a code owner November 24, 2023 16:34
@motjuste
Copy link
Author

I am happy to write tests for this, but would need guidance to get started.

@orfeas-k
Copy link
Contributor

Hi @motjuste, does this PR try to resolve #287?

@motjuste
Copy link
Author

motjuste commented Nov 27, 2023

Hi @motjuste, does this PR try to resolve #287?

Not intentionally, but yes, I also faced an issue that was because of not handling the None response.

@orfeas-k
Copy link
Contributor

That's great. Thank you for taking the time to try and fix this. Could you add a unit test then that replicates the situtation as described in the issue #287 (comment)? This way, we 'll make sure that the fix you 've sent makes what we expect it to.

You could do that by adding another fixture for the specific case and then adding it as a parameter to the tests we already have (first and second).

@motjuste
Copy link
Author

Thanks for pointing me to the tests. I'll update the PR with them.

@motjuste
Copy link
Author

motjuste commented Nov 27, 2023

Added a new mock fixture (sorry for the verbose name, suggestions welcome), and then I added it to the two parameterised tests.

I can't seem to check the CI outputs attached to this PR below. I am new to GitHub Actions ... is there a way for me to get the test running in GitHub without having to set everything up on my laptop?

@orfeas-k
Copy link
Contributor

Hi @motjuste, we discussed this issue with our team internally and decided that we should start allowing PRs from external forks to run workflows (issue canonical/bundle-kubeflow#764). We will look to implement this in the near future but at the moment, this is not possible (for security reasons as mentioned in the issue).

For that reason, we will hold this PR until that issue is resolved, so we can properly test and merge it. Once again, thank you for taking the time to contribute to our project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants