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

Feature: create service.targetPort parameter or default to nextcloud.containerPort for targetPort in service template #384

Closed
jessebot opened this issue Apr 24, 2023 · 3 comments · Fixed by #386
Assignees
Labels
enhancement New feature or request

Comments

@jessebot
Copy link
Collaborator

jessebot commented Apr 24, 2023

Description of the change

I feel like we should allow users to set a service.targetPort :

service:
type: ClusterIP
port: 8080
loadBalancerIP: nil
nodePort: nil

It might also make sense to default to nextcloud.containerPort here if service.targetPort is not set or not chosen for implementation:

Benefits

This allows users to use different container ports, such as 9000, to have a more direct connection to the ingess-nginx controller directly, instead of using the nginx container.

Possible drawbacks

Not sure if we should also set port name fields? We use http a lot and this could get confusing?

Additional information

Happy to submit a PR for this. It would also be part of the work to implement #367 down the line.

@jessebot jessebot added the enhancement New feature or request label Apr 24, 2023
@jessebot jessebot self-assigned this Apr 24, 2023
@jessebot jessebot changed the title Feature: create service.targetPort parameter or default to nextcloud.containerPort for target port in service template Feature: create service.targetPort parameter or default to nextcloud.containerPort for targetPort in service template Apr 24, 2023
@jessebot
Copy link
Collaborator Author

Added #386 as a draft PR, because I think this is a good idea, but in the PR, I just decided to default to nextcloud.containerPort for the service targetPort to reduce complexity. Open to better ways to do this.

@jessebot
Copy link
Collaborator Author

jessebot commented Apr 24, 2023

relates to the following PRs, so I'll close my draft if these get merged first:

@provokateurin
Copy link
Member

@jessebot would you mind closing or updating the other PRs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants