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

Add New Port Mappings for Helm chart #352

Merged
merged 6 commits into from
Feb 20, 2025

Conversation

peixian
Copy link
Contributor

@peixian peixian commented Feb 14, 2025

  • The stats port 8199 is open by default, and it should probably be closed.

  • Media ports are not open by default on the helm chart, fixes that

  • Changes the service type to an external load balancer type so that a default helm deployment creates an external IP.

  • Also fixes the docker build pipeline for PRs to only build, and not push

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 14, 2025
@peixian peixian force-pushed the peixian/add-external-loadbalancer branch 2 times, most recently from 549a74f to 4ce8417 Compare February 14, 2025 19:25
@peixian peixian force-pushed the peixian/add-external-loadbalancer branch from 4ce8417 to 515e365 Compare February 14, 2025 19:28
# http_proxy_port: 8080
# https_proxy_port: 8443
# jabber_proxy_port: 8222
# media_proxy_port: 7777
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this one as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, these are left as examples for other users

{{- if .Values.service.media_proxy_port }}
- port: {{ .Values.service.media_proxy_port }}
targetPort: 7777
protocl: TCP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. typo? How is YAML passing?

@peixian peixian force-pushed the peixian/add-external-loadbalancer branch from 25486ea to b4fe2a4 Compare February 19, 2025 22:59
{{- if .Values.service.media_port }}
- port: {{ .Values.service.media_port }}
targetPort: 587
protocl: TCP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too.

Copy link
Contributor

@eozturk1 eozturk1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming you have tested this version and confirmed proxy works!

@@ -0,0 +1,38 @@
name: Docker Image Build for PRs

# Run on every push to main + weekly, Sunday @ midnight
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the comment accurate?

@peixian peixian merged commit fc0bb96 into WhatsApp:main Feb 20, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants