feat: extra headers during initialization #668
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
i agree CLA
ср, 25 сент. 2024 г. в 00:06, cla-bot[bot] ***@***.***>:
… Thank you for your pull request and welcome to the Trino community. We
require contributors to sign our Contributor License Agreement
<https://github.com/trinodb/cla/raw/master/Trino%20Foundation%20Individual%20CLA.pdf>,
and we don't seem to have you on file. Continue to work with us on the
review and improvements in this PR, and submit the signed CLA to
***@***.*** Photos, scans, or digitally-signed PDF files are all
suitable. Processing may take a few days. The CLA needs to be on file
before we merge your changes. For more information, see
https://github.com/trinodb/cla
—
Reply to this email directly, view it on GitHub
<#668 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGJDWXOPFHMLDPYNTORWRDTZYHH6DAVCNFSM6AAAAABOZETB72VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZSGM4DMNJVGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@aderihoJD please fill in the description of this pull request. There's an open discussion in trinodb/trino#15826 about allowing clients to send any HTTP headers. It would be better to discuss specific use cases. Similar changes were suggested in both the Trino Go client, and the Python client, and both were rejected, because no sufficient use case was provided. |
|
@regadas could u review it? |
|
Please describe your use case, per my previous comment. |
yes, sure. i left a description already. and in our case - we need an extra header to allow us communicate with trino inside cluster. that's why we need a X-Forwarded-Proto: https header for each request to trino. |
|
Can you add a test that would verify this? |
not sure. it's a specific case during communicate inside k8s cluster. we need to setup it to verify |
|
@nineinchnick do we need smth from my side? |
|
Yes, this needs any kind of tests. A unit test would suffice, but we only have integration tests here. @regadas PTAL |
|
@aderihoJD @nineinchnick sorry for dropping the ball a bit on this; Added a test to ensure propagation. |
|
Please note our commit guidelines when sending PRs @aderihoJD .. and also when merging @regadas See https://trino.io/development/process#pull-request-and-commit-guidelines- and https://trino.io/development/process#pull-request-review-and--merge |
|
@regadas could we make a release with this changes? |
|
@aderihoJD Sorry for the delay; https://github.com/trinodb/trino-js-client/releases/tag/v0.2.4 |
Unfortunately, this feature #568 not fully provide possibility to apply extra headers for each request to Trino. They apply only for the first one (query) and we lose them later, in next() method. If we'll add extraHeaders during client initialization, they will apply for each request.
p.s. pretty urgent, if possible