Skip to content

feat: extra headers during client initialization #666

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

Closed

Conversation

aderihoJD
Copy link
Contributor

@aderihoJD aderihoJD commented Sep 24, 2024

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

Copy link

cla-bot bot commented Sep 24, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: aderihoilya.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

Copy link
Contributor

@regadas regadas left a comment

Choose a reason for hiding this comment

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

Hi @aderihoJD Thanks for this.

Can you add a test for it?

@aderihoJD
Copy link
Contributor Author

Hi @aderihoJD Thanks for this.

Can you add a test for it?
@regadas not sure. I see a possible suite, that we can check that each request will apply with certain extra header, which we provide during client initialization. But we can't access request method.
Maybe u have some proposals about possible test suite?

@mosabua
Copy link
Member

mosabua commented Sep 24, 2024

@aderihoJD please provide a signed CLA as well

@aderihoJD
Copy link
Contributor Author

@aderihoJD please provide a signed CLA as well

i had to create a new one, as i did whole flow from another email #668

@mosabua
Copy link
Member

mosabua commented Sep 24, 2024

@aderihoJD please provide a signed CLA as well

i had to create a new one, as i did whole flow from another email #668

Your CLA probably still has to be processed. Stay tuned. Also if you rebase your commit you can change author as well so theoretically you dont need a new PR. But either is fine and we need to wait until CLA is processed. Your github username will show up in https://github.com/trinodb/cla/blob/master/contributors

@aderihoJD
Copy link
Contributor Author

@mosabua i appeared in this list https://github.com/trinodb/cla/blob/master/contributors
what is the next step?

@nineinchnick
Copy link
Member

@cla-bot check

Copy link

cla-bot bot commented Oct 14, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: aderihoilya.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

Copy link

cla-bot bot commented Oct 14, 2024

The cla-bot has been summoned, and re-checked this pull request!

@nineinchnick
Copy link
Member

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.

@nineinchnick
Copy link
Member

BTW the CLA bot check is not passing, because the commit is authored by aderihoilya, which is not matching @aderihoJD username/email.

@aderihoJD
Copy link
Contributor Author

@nineinchnick i created a new MR with correct user #668 - it's not a duplicate

@nineinchnick
Copy link
Member

So can this one be closed then?

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

Successfully merging this pull request may close these issues.

4 participants