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

Update docker nginx config to not force ipv6 #28729

Closed
wants to merge 4 commits into from

Conversation

yxie2023
Copy link

@yxie2023 yxie2023 commented Dec 12, 2024

ipv6 support is automatically added via nginx docker's 10-listen-on-ipv6-by-default.sh.

This patch allows element-web to run on system with or without ipv6 enabled.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

ipv6 support is automatically added via nginx docker's 10-listen-on-ipv6-by-default.sh.

This patch allows element-web to run on system with or without ipv6 enabled.
@yxie2023 yxie2023 requested a review from a team as a code owner December 12, 2024 16:46
@CLAassistant
Copy link

CLAassistant commented Dec 12, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ yxie2023
❌ x


x seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Dec 12, 2024
@spaetz
Copy link

spaetz commented Dec 15, 2024

Vote against from me :-). Sorry, my nginx does not come throough docker and I don't believe disabling IPv6 by default makes sense nowadays. Add a comment to tell people to comment this out if they don't use IPv6, but no one should be just using this config without going through it and adapting it as they need fit anyway. Don't just disable IPv6.

@t3chguy
Copy link
Member

t3chguy commented Dec 17, 2024

Vote against from me :-). Sorry, my nginx does not come throough docker and I don't believe disabling IPv6 by default makes sense nowadays. Add a comment to tell people to comment this out if they don't use IPv6, but no one should be just using this config without going through it and adapting it as they need fit anyway. Don't just disable IPv6.

The config isn't intended as an example, but the config we use for our Docker image only. @yxie2023 could you move the nginx dir into a docker dir to make that clearer, then apply the same change you made therein?

@yxie2023
Copy link
Author

done @t3chguy. let me know if there's anything else. thank you!

@t3chguy t3chguy added the T-Task Tasks for the team like planning label Dec 18, 2024
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Looks sane to me, thanks

@t3chguy t3chguy enabled auto-merge December 18, 2024 09:45
@t3chguy t3chguy changed the title Update default.conf Update nginx config to not force ipv6 Dec 18, 2024
@t3chguy t3chguy changed the title Update nginx config to not force ipv6 Update docker nginx config to not force ipv6 Dec 18, 2024
@t3chguy
Copy link
Member

t3chguy commented Dec 18, 2024

@yxie2023 you would need to sign the CLA if you want your contribution to be merged. Also looks like Sonarcloud won't scan your own develop branch so you'd need to contribute from a different branch name.

@yxie2023
Copy link
Author

I signed the CLA.

Resubmitting from "contrib" branch: #28771

@t3chguy t3chguy closed this Dec 19, 2024
auto-merge was automatically disabled December 19, 2024 09:30

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants