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

Set default for nextcloud.containerPort in values.yaml and update YAML templates to use it everywhere #386

Merged
merged 2 commits into from
Apr 29, 2023

Conversation

jessebot
Copy link
Collaborator

@jessebot jessebot commented Apr 24, 2023

Pull Request

Description of the change

Reworked this PR to include other changes from two other PRs to move along these changes faster in bulk.

  • Sets default port for nextcloud.containerPort to be 80 in values.yaml.
  • Changes the service targetPort for the nextcloud service to be the nextcloud.containerPort.
  • Changes all http ports in the deployment template to use nextcloud.containerPort instead (includes probes)

Benefits

allows users to set a custom container port like 9000 and have it be referenced in the service.

Possible drawbacks

unsure, open to suggestion

Applicable issues

Additional information

Checklist

@jessebot
Copy link
Collaborator Author

relates to the following issues:

@provokateurin, I noticed you said in #278 (comment) that we should use a default value. Did you mean for nextcloud.containerPort? I can set nextcloud.containerPort in values.yaml. Right now it's commented out here and set to 8080, should it be uncommented and set to 8080 or 80:

# containerPort: 8080

Happy to do the work to get all three PRs moved forward :) Also happy to add any docs you think might be handy

@jessebot jessebot force-pushed the feature/use-containerport-as-targetport branch from 3db3077 to d7c4941 Compare April 24, 2023 15:46
@jessebot
Copy link
Collaborator Author

jessebot commented Apr 24, 2023

Actually, I could forgo this PR entirely, as it looks like #316 already updates the service.yaml. Will close this if we can merge the other one.

@jessebot jessebot changed the title Use nextcloud.containerPort as service targetPort Set default for nextcloud.containerPort in values.yaml and update YAML templates to use it everywhere Apr 29, 2023
@jessebot jessebot force-pushed the feature/use-containerport-as-targetport branch 2 times, most recently from 142e942 to 7b8ba32 Compare April 29, 2023 00:38
…e nextcloud.containerPort in service and deployment templates

Signed-off-by: Jesse Hitch <jessebot@linux.com>
Signed-off-by: Jesse Hitch <jessebot@linux.com>
@jessebot jessebot force-pushed the feature/use-containerport-as-targetport branch from 7b8ba32 to 3d492ca Compare April 29, 2023 00:41
@jessebot jessebot marked this pull request as ready for review April 29, 2023 00:41
@jessebot
Copy link
Collaborator Author

Have fixed up this PR because the other PRs that do something similar seem to have gone stale

@jessebot jessebot merged commit a215de8 into main Apr 29, 2023
@delete-merged-branch delete-merged-branch bot deleted the feature/use-containerport-as-targetport branch April 29, 2023 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants