Skip to content

Conversation

0xbxmb
Copy link
Contributor

@0xbxmb 0xbxmb commented Apr 12, 2022

@bryanlatten
Copy link
Contributor

@bombinmybag thanks for your contributions, do you want to add runtime enablement for these placeholders?

@ingluisjimenez
Copy link
Contributor

@bombinmybag you can add a script here https://github.com/behance/docker-nginx/tree/master/container/root/etc/cont-init.d to enable brotli based on specific environment variable configuration

@0xbxmb
Copy link
Contributor Author

0xbxmb commented Apr 18, 2022

@ingluisjimenez could you please check the update when you have a chance?

@0xbxmb 0xbxmb changed the title Nginx: Placeholders required for add Brotli support were added. Nginx: brotli support Apr 19, 2022
@0xbxmb
Copy link
Contributor Author

0xbxmb commented Apr 19, 2022

@ingluisjimenez could you please check again, when you have a chance?
#72

@0xbxmb 0xbxmb requested a review from ingluisjimenez April 19, 2022 21:26
@0xbxmb
Copy link
Contributor Author

0xbxmb commented Apr 21, 2022

@ingluisjimenez are my changes causing this?
Can someone help me with validating it?

image

@0xbxmb
Copy link
Contributor Author

0xbxmb commented Jun 9, 2022

I applied 10-nginx-config.sh manually to test – seems like the modules are in the right place and the configuration is correct.

Screen Shot 2022-06-08 at 18 21 38

Screen Shot 2022-06-08 at 18 19 47

image

image

A response with Brotli:

image

@0xbxmb 0xbxmb force-pushed the feat-nginx-br-support branch from b30dfdf to d6bcb9b Compare June 9, 2022 15:48
Dockerfile Outdated
apt-get install -yqq --no-install-recommends \
nginx-light \
ca-certificates \
libnginx-mod-brotli \
Copy link

Choose a reason for hiding this comment

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

does it matter that we do not ask for a specific version here? or do we want to pin to a specific version and then tag the docker image with the same?

Copy link
Contributor Author

@0xbxmb 0xbxmb Jun 9, 2022

Choose a reason for hiding this comment

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

@kangman thank you that's a good question. I'm not sure that we need to, but if we will, should we also specify version for nginx-light so we can control the pair? @ingluisjimenez what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine with either way, but if we can tag then that seems more controller and a better idea

Copy link
Contributor Author

@0xbxmb 0xbxmb Aug 9, 2022

Choose a reason for hiding this comment

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

@ingluisjimenez @kangman could you please review the updated version.

@0xbxmb 0xbxmb requested a review from kangman June 9, 2022 17:13
Dockerfile Outdated
apt-get update -yqq && \
apt-get install -yqq --no-install-recommends \
nginx-light \
nginx-light=1.22.0-* \
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the same version we currently have? or a different one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ingluisjimenez I think it will pull latest on build.
Version on my vm is nginx-light | 1.20.1

Copy link

Choose a reason for hiding this comment

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

how does it pull 1.20.1 if it is pinned to 1.22.0-*?

Copy link
Contributor Author

@0xbxmb 0xbxmb Aug 10, 2022

Choose a reason for hiding this comment

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

@kangman I mean – currently I have 1.20.1-2+ubuntu20.04.1+deb.sury.org+1 on my machine.
When I try to build the image with the version

    apt-get install -yqq --no-install-recommends \
        nginx-light=1.20.1-* \
        ca-certificates \
        libnginx-mod-brotli=1.20.1-* \

i'm getting the error:

#4 52.60 E: Version '1.20.1-*' for 'nginx-light' was not found
#4 52.60 E: Version '1.20.1-*' for 'libnginx-mod-brotli' was not found

So, when I run apt-cache madison nginx-light I see that 1.22.0 version was available.

# apt-cache madison nginx-light
nginx-light | 1.22.0-1+ubuntu20.04.1+deb.sury.org+1 | http://ppa.launchpad.net/ondrej/nginx/ubuntu focal/main arm64 Packages
nginx-light | 1.18.0-0ubuntu1.3 | http://ports.ubuntu.com/ubuntu-ports focal-updates/universe arm64 Packages
nginx-light | 1.18.0-0ubuntu1.3 | http://ports.ubuntu.com/ubuntu-ports focal-security/universe arm64 Packages
nginx-light | 1.17.10-0ubuntu1 | http://ports.ubuntu.com/ubuntu-ports focal/universe arm64 Packages

that's why it's specified there.

@kangman please let me know if you are ok with it.
ps: changelog

@adobejmong adobejmong force-pushed the feat-nginx-br-support branch 3 times, most recently from 4af3c17 to 477186b Compare August 17, 2022 23:48
* Parameterize NGINX_VERSION
* Rename SERVER_ENABLE_BROTLI to SERVER_ENABLE_NGX_BROTLI
* Add SERVER_SHOW_NGINX_CONF to help with debugging
* Fix sed for SERVER_ENABLE_NGX_HTTP_JS
* Update nginx.conf with the brotli modules and configuration
* Add brotli goss test
* Add dgoss test for testing the brotli module
@adobejmong adobejmong force-pushed the feat-nginx-br-support branch from 477186b to 1a8764f Compare August 17, 2022 23:51
#
# Should be the last entry in this script to ensure that all prior
# modifications have been applied
if [[ $SERVER_SHOW_NGINX_CONF ]];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🔥

@adobejmong adobejmong merged commit 615bf0e into behance:master Aug 18, 2022
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.

6 participants