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

feat: frontend webpack proxy support zstd encoding #29822

Closed
wants to merge 1 commit into from

Conversation

rebareba
Copy link

@rebareba rebareba commented Aug 1, 2024

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

@dosubot dosubot bot added the change:frontend Requires changing the frontend label Aug 1, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@rusackas
Copy link
Member

rusackas commented Aug 1, 2024

Thank you so much for this! Looks like it's working fine. It might need a rebase to pick up a CI fix from another PR, and it looks like it has some linting issues, which can be resolved with our pre-commit hooks.

Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following:

pip3 install -r requirements/development.txt
pre-commit install
A series of checks will now run when you make a git commit.

Alternatively it is possible to run pre-commit by running pre-commit manually:

pre-commit run --all-files

@rusackas
Copy link
Member

rusackas commented Aug 1, 2024

Pushed a commit to (hopefully) fix the linting issue

@rusackas
Copy link
Member

rusackas commented Aug 1, 2024

Not sure why Docker is failing here (cc @mistercrunch )

Good news is, npm run dev-server works as expected again, hooray!

When I go to port 8088, however, it doesn't look like the webpack build happened, and the frontend isn't present.

@mistercrunch
Copy link
Member

@dosubot what is zstd and why do we need it?

@mistercrunch
Copy link
Member

Not sure why Docker is failing here (cc @mistercrunch )

I think we're hitting memory limits in GHA, it seems we've been right against the edge on it and I'm afraid PRs are going to fail randomly more and more until we address it. I discussed it here #29771 (comment), and I'm a bit at a loss for a solution.

@rebareba rebareba force-pushed the responseEncoding-zstd branch from a3430f4 to f62a3a4 Compare August 2, 2024 02:32
@rebareba
Copy link
Author

rebareba commented Aug 2, 2024

Docker failed This looks like we needs to install zstd in System. The npm package simple-zstd use system command zstd to compression and decompression .
apt-get -y install zstd

@rusackas
Copy link
Member

Superseded by #30034
Thanks for figuring this out @rebareba !

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

Successfully merging this pull request may close these issues.

3 participants