-
Notifications
You must be signed in to change notification settings - Fork 2
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
Use LoadBalancer Service #36
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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.
Two general comments:
- I think we could mention in the README that it is possible to expose UPF, but LB is required for that. We could also have an integration test validating the exposure. WDYT?
- Please add docstrings :)
Sure, I can add that to the README. I'd be happy to give integration tests a shot, but at this point I'm not quite sure what that would look like. I'll have to tackle those when I get back on Wednesday.
Happy to add docstrings where you think they would be useful. I'm not a fan of docstrings on internal functions where the docstring just reiterates the function name. i.e.
The above seems like a waste, but maybe there is something you'd like me to be more explicit about? |
This comment was marked as outdated.
This comment was marked as outdated.
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.
Needs to be rebased before merging. LGTM.
Description
Exposes an external LoadBalancer service for the UPF, similar to canonical/sdcore-amf-k8s-operator#24 for the AMF
Checklist: