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

Brotli compression support #379

Merged
merged 18 commits into from
Feb 10, 2024
Merged

Brotli compression support #379

merged 18 commits into from
Feb 10, 2024

Conversation

Theodlz
Copy link
Collaborator

@Theodlz Theodlz commented Feb 6, 2024

Don't review yet, just a draft

tools/fill_conf_values.py Outdated Show resolved Hide resolved
tools/fill_conf_values.py Outdated Show resolved Hide resolved
@Theodlz Theodlz requested a review from stefanv February 7, 2024 05:18
@Theodlz
Copy link
Collaborator Author

Theodlz commented Feb 7, 2024

@stefanv I need to make some changes to the GA (like go back to the real baselayer app template not my fork) before merging anything, but this works very well! The code snippet from the GA can be reused in the SkyPortal dockerfile to install nginx with brotli :). I will also add some documentation before merging, but re-requesting a review so you can see the new code.

.github/workflows/test.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@stefanv stefanv left a comment

Choose a reason for hiding this comment

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

Looks good! A few small questions.

doc/setup.md Outdated Show resolved Hide resolved
tools/fill_conf_values.py Outdated Show resolved Hide resolved
tools/fill_conf_values.py Outdated Show resolved Hide resolved
@Theodlz
Copy link
Collaborator Author

Theodlz commented Feb 9, 2024

@stefanv updated the GA and the docs to install from the ppa, and updated the python logic to find the nginx brotli module. This is solid, works well on both dynamic and static installations (and on both windows and mac).

Let me know what you think. If we want this to be tested, we will have to update the baselayer template app with a test (like the one I added to the fork pinned here for testing). So if you are happy with these changes, let me know and I'll pin the normal template app again and we can merge :)

@stefanv stefanv changed the title WIP - Brotli support Brotli compression support Feb 10, 2024
@stefanv
Copy link
Contributor

stefanv commented Feb 10, 2024

I like it! Let's also merge the test on BL.

@Theodlz
Copy link
Collaborator Author

Theodlz commented Feb 10, 2024

Great! I went back to git cloning the real template app in the GA. I'll open the PR on the template app ASAP.

@Theodlz Theodlz merged commit 1160f75 into main Feb 10, 2024
4 checks passed
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.

2 participants