search

digitalocean / nginxconfig.io

⚙️ NGINX config generator on steroids 💉
https://do.co/nginxconfig
MIT License
27.42k stars 2.01k forks source link

Update how HTTP/2 is enabled #456

Closed Cyb3r-Jak3 closed 5 months ago

Cyb3r-Jak3 commented 8 months ago

Type of Change

What issue does this relate to?

Resolves #451

What should this PR do?

Changes how HTTP2 is enabled with

What are the acceptance criteria?

Verify that new HTTP/2 directive matches the docs.

Previous firefox_AIwZwD86ZG

With changes firefox_uKflHkIgPI

MattIPv4 commented 7 months ago

👀 I'm somewhat concerned this is going to be very breaking for most users? This directive isn't even in stable yet, and causes the entire configuration to error out. It was landed in 1.25.1 which is mainline, whereas 1.24.x is considered stable: https://nginx.org/en/CHANGES

NGINXConfig generally tries to stay as backwards compatible as possible, so until deprecated directives are completely removed, we generally keep using them to support the most versions possible.

MattIPv4 commented 7 months ago

I think perhaps under the global NGINX tab, we need options to toggle what directives are being used? In the same 1.25.1 release, the ssl directive was also removed which NGINXConfig currently uses during the installation flow. Then again, perhaps that installation flow could be revisited to not require disabling SSL temporarily.

Cyb3r-Jak3 commented 7 months ago

Ah, I was following the mainline changes, not stable.

I agree that a toggle, maybe it could change between mainline and stable? I know that HTTP/3 support is now also in mainline version, so it would no longer require the custom build warning that is currently visible.

MattIPv4 commented 7 months ago

Yeah, that sounds like a good way to approach it. Probably just a checkbox under the global NGINX tab for "enable NGINX mainline support" or smth.

Cyb3r-Jak3 commented 7 months ago

Should I include that as part of this PR or make a seperate one?

MattIPv4 commented 7 months ago

I wouldn't worry about the SSL stuff in this PR, but adding the checkbox in this PR to allow the http2 change to be toggled would be great :)

MattIPv4 commented 5 months ago

I'm going to close this PR out as there hasn't been any movement in a while. You're welcome to re-open or create a new one if you find the time to work on the changes :)