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 bridge port to Pod and Service manifests in Helm Chart #802

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Hy3n4
Copy link

@Hy3n4 Hy3n4 commented Jul 31, 2023

Hey there 👋 ,
I had this fight with the Hookshot installed via Helm Chart and this is the result. More info in matrix thread here

I hope I did not forgot anything and this PR is valid.

@Hy3n4 Hy3n4 requested a review from a team as a code owner July 31, 2023 15:32
@@ -48,6 +48,9 @@ containers:
mountPath: "/data"
{{- end }}
ports:
- name: bridge
containerPort: {{ .Values.hookshot.config.bridge.port }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Heya, do you know what the bridge port corresponds to. This isn't the appservice (below)?

Copy link
Author

Choose a reason for hiding this comment

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

Originally I thouth so, but I'm not sure, TBH. There is a port 9993 defined in the bridge configuration section. But in the service and container definitions are used hardcoded ports which does not correspond with bridge port. 🤷‍♂️

Copy link
Author

Choose a reason for hiding this comment

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

So I've been digging more and the ports are really different. The appservice here is probably serving widget stuff, but the bridge itself is running on port defined in .Values.hookshot.config.bridge.port. Therefore, there must be another service for it.
Here is what ports are actually listening in the container:
image

And Here in the values.yaml, there is appservice as listener.
So I think this PR is legit. 🤔 At least I couldn't make the hookshot work without these changes.

Copy link
Author

Choose a reason for hiding this comment

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

Hey, It's been a while. Did you manage to get into this? I want to use the Helm chart installation, but I cannot, since the bridge port is not propagated on Pod or even available in the Service.

If I am doing something wrong, I would like to know what 🤷‍♂️
Thanks

Copy link

@rhizoet rhizoet Jun 17, 2024

Choose a reason for hiding this comment

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

Yes, can this please merged? We need to have Port 9993 opened in the cluster, so that we can reach it with the registration of synapse. Otherweise the helm chart ist useless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants