-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
fix(async-queries): make global async. queries cookie SameSite option configurable #21185
fix(async-queries): make global async. queries cookie SameSite option configurable #21185
Conversation
There was a problem hiding this 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!
Thanks for the fix, and sorry it hasn't gotten more attention. Trying to get it routed on a path to success! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! One minor nit, but happy to get this merged asap if you can update that type annotation!
superset/config.py
Outdated
@@ -1282,6 +1282,7 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument | |||
GLOBAL_ASYNC_QUERIES_REDIS_STREAM_LIMIT_FIREHOSE = 1000000 | |||
GLOBAL_ASYNC_QUERIES_JWT_COOKIE_NAME = "async-token" | |||
GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SECURE = False | |||
GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SAMESITE = None # One of [None, 'None', 'Lax', 'Strict'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it's generally convention to add these as type annotations:
GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SAMESITE = None # One of [None, 'None', 'Lax', 'Strict'] | |
GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SAMESITE: Optional[Literal['None', 'Lax', 'Strict']] = None |
…al-async-queries-cookie
Codecov Report
@@ Coverage Diff @@
## master #21185 +/- ##
=======================================
Coverage 67.10% 67.11%
=======================================
Files 1869 1869
Lines 71609 71612 +3
Branches 7822 7822
=======================================
+ Hits 48056 48059 +3
Misses 21526 21526
Partials 2027 2027
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for adding this option
This PR fixes #21183
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION