Skip to content

Don't require adding the https port in additionalExposedPorts when https is enabled #322

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

Merged
merged 2 commits into from
Apr 29, 2025

Conversation

cccs-nik
Copy link
Member

The changes below are a subset of the changes seen in #300 but the PR hasn't seen activity in over a month so I thought it might make sense to create this small PR. The changes in this PR are also independent of the other changes proposed by #300 to support internal TLS.

@cla-bot cla-bot bot added the cla-signed label Mar 25, 2025
@nineinchnick
Copy link
Member

See the test failures:

 Error: INSTALLATION FAILED: 2 errors occurred:
Error: failed installing charts: failed processing charts
	* Service "trino-9qjh10dbwo-trino" is invalid: [spec.ports[3].name: Duplicate value: "https", spec.ports[3]: Duplicate value: core.ServicePort{Name:"", Protocol:"TCP", AppProtocol:(*string)(nil), Port:8443, TargetPort:intstr.IntOrString{Type:0, IntVal:0, StrVal:""}, NodePort:0}]
	* Deployment.apps "trino-9qjh10dbwo-trino-coordinator" is invalid: spec.template.spec.containers[0].ports[4].name: Duplicate value: "https"

@cccs-nik
Copy link
Member Author

Thanks for the heads-up @nineinchnick, the port was indeed duplicated from additionalExposedPorts in the test file. I removed it and it looks good now.

@nineinchnick nineinchnick added the breaking-change Potentially breaking changes that require user action. Will be highlighted in release notes. label Mar 25, 2025
@nineinchnick nineinchnick changed the title include https port where needed when https is enabled Don't require adding the https port in additionalExposedPorts when https is enabled Apr 6, 2025
@nineinchnick
Copy link
Member

Please squash commits and I can merge this.

@nineinchnick nineinchnick merged commit 81c5a13 into trinodb:main Apr 29, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Potentially breaking changes that require user action. Will be highlighted in release notes. cla-signed
Development

Successfully merging this pull request may close these issues.

2 participants