search

anycable / anycable-go

AnyCable real-time server
https://anycable.io
MIT License
375 stars 65 forks source link

feat(cli): proxy-cookies flag to filter cookies #157

Closed rafaelrubbioli closed 2 years ago

rafaelrubbioli commented 2 years ago

fixes https://github.com/anycable/anycable-go/issues/156

What is the purpose of this pull request?

Add a way to filter the cookies that are sent, to simplify and remove unneeded cookies

What changes did you make? (overview)

I added a flag --proxy-cookies that, if set, will filter the cookies that are sent to the server

Is there anything you'd like reviewers to focus on?

Please check if I did everything that was needed. I'd be happy to fix any problems and hear any suggestions 😄

Checklist

Q: do i need to add a changelog entry?

rafaelrubbioli commented 2 years ago

@palkan Sure, will work on it!

rafaelrubbioli commented 2 years ago

@palkan Thanks for the tip! That was a great suggestion! :rocket:

rafaelrubbioli commented 2 years ago

@palkan If everything is fine can you add the hacktoberfest-accepted label? Thanks!

palkan commented 2 years ago

Thanks!

Let's fix the linting errors and we're good to merge it.

rafaelrubbioli commented 2 years ago

I'll try to fix all errors, but there was an error on test-conformance, and I'm not really sure where to fix it 🤔

palkan commented 2 years ago

there was an error on test-conformance, and I'm not really sure where to fix it

This is an integration test defined here: https://github.com/anycable/anyt/blob/ee8c622ff1b3c0431435a65e1047632c078208ba/lib/anyt/tests/request/connection_test.rb#L32

From the logs, we seen that no cookies are proxied (though we do not set proxy-cookies); looks like a bug.

UPD: See https://go.dev/play/p/zToXcc6dBvP

rafaelrubbioli commented 2 years ago

there was an error on test-conformance, and I'm not really sure where to fix it

This is an integration test defined here: https://github.com/anycable/anyt/blob/ee8c622ff1b3c0431435a65e1047632c078208ba/lib/anyt/tests/request/connection_test.rb#L32

From the logs, we seen that no cookies are proxied (though we do not set proxy-cookies); looks like a bug.

UPD: See go.dev/play/p/zToXcc6dBvP

Good catch! Thats why we always have tests 😅 I think it's fixed now

palkan commented 2 years ago

Perfect! Thanks!