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: add support for response compression #723

Merged
merged 44 commits into from
May 26, 2024

Conversation

Rutik7066
Copy link
Contributor

@Rutik7066 Rutik7066 commented Apr 11, 2024

/claim #602

We have added middleware to compress the response.

TODO

router/core/header_rule_engine_test.go Outdated Show resolved Hide resolved
@Rutik7066
Copy link
Contributor Author

Hi @StarpTech If you like I can claim #674 in this repo.

@Rutik7066 Rutik7066 marked this pull request as ready for review April 13, 2024 20:48
Copy link

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 29, 2024
@Rutik7066 Rutik7066 marked this pull request as draft May 1, 2024 13:22
@Rutik7066 Rutik7066 marked this pull request as ready for review May 1, 2024 13:22
@Rutik7066 Rutik7066 requested a review from StarpTech May 24, 2024 08:58
@Rutik7066
Copy link
Contributor Author

@StarpTech changes applied. Github action Router CI is falling don't know why. It is not related to my code.
After updating branch.

@StarpTech
Copy link
Contributor

@Rutik7066 #723 (comment)

@Rutik7066 Rutik7066 requested a review from StarpTech May 24, 2024 14:47
router/core/request_compression_test.go Outdated Show resolved Hide resolved
router-tests/request_compression_test.go Outdated Show resolved Hide resolved
@Rutik7066 Rutik7066 requested a review from StarpTech May 24, 2024 16:52
@Aenimus
Copy link
Member

Aenimus commented May 25, 2024

@Rutik7066 can you run make sync-go-workspace from root please?

@Rutik7066
Copy link
Contributor Author

make sync-go-workspace

@Aenimus Done.

@Rutik7066
Copy link
Contributor Author

@StarpTech we can merge this now. all changes are applied. please check.

@StarpTech
Copy link
Contributor

StarpTech commented May 26, 2024

Hi @Rutik7066,

Thanks for your work on the PR! I noticed a couple of things that need to be addressed before we can merge it.

First, the PR title and test files mention "request compression," but your code actually addresses "response compression." Please update the wording in the test files and PR to reflect "response compression" instead.

Also, the AllowContentEncoding middleware you added checks if a request with a specific encoding can be decoded, but you didn't include support for it. Since it currently doesn't do anything, please remove this middleware.

Lastly, I want to remind you that the bounty is intended to be a solo challenge, and the requirements are quite clear. The level of help you're seeking goes beyond what's expected for this challenge.

Once you've made these changes, we can proceed with merging the PR. Thanks for your understanding!

@Rutik7066
Copy link
Contributor Author

Ok I will update.

@StarpTech StarpTech changed the title feat: add support for request compression feat: add support for response compression May 26, 2024
@Rutik7066 Rutik7066 changed the title feat: add support for response compression feat: add support for response compression. May 26, 2024
@Rutik7066 Rutik7066 changed the title feat: add support for response compression. feat: add support for response compression May 26, 2024
@Rutik7066 Rutik7066 requested a review from StarpTech May 26, 2024 12:42
@Rutik7066
Copy link
Contributor Author

Hi @StarpTech all changes applied.

@Rutik7066
Copy link
Contributor Author

@StarpTech I hope everything is perfect now to merge.

Copy link
Contributor

@StarpTech StarpTech left a comment

Choose a reason for hiding this comment

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

LGTM

@StarpTech StarpTech merged commit a6c6ac4 into wundergraph:main May 26, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants