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

Enable user headers in the reflect call in client.connect #3242

Closed
wants to merge 3 commits into from

Conversation

lirao
Copy link

@lirao lirao commented Aug 1, 2023

What?

Add a parameter reflectMetadata in client.connect to add user-provided headers in the reflect call.

Why?

Some GRPC servers require a header to authenticate any call (including reflection). The invoke command covers this use case, but the reflect command that's bundled in the connect command does not, which leads to authentication errors.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make ci-like-lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Closes #3241

Rao Li added 2 commits August 1, 2023 12:18
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Rao Li seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov-commenter
Copy link

Codecov Report

Merging #3242 (f416f1f) into master (7184786) will decrease coverage by 0.06%.
The diff coverage is 66.66%.

❗ Current head f416f1f differs from pull request most recent head da66ac8. Consider uploading reports for the commit da66ac8 to get more accurate results

@@            Coverage Diff             @@
##           master    #3242      +/-   ##
==========================================
- Coverage   73.21%   73.16%   -0.06%     
==========================================
  Files         259      257       -2     
  Lines       19899    19906       +7     
==========================================
- Hits        14569    14564       -5     
- Misses       4406     4412       +6     
- Partials      924      930       +6     
Flag Coverage Δ
ubuntu 73.16% <66.66%> (+0.01%) ⬆️
windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
js/modules/k6/grpc/client.go 84.03% <66.66%> (-0.66%) ⬇️

... and 8 files with indirect coverage changes

Copy link
Collaborator

@codebien codebien left a comment

Choose a reason for hiding this comment

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

Hey @lirao,
thanks for your contribution. 🙏 Can you please sign the CLA so we can accept your contribution?

@lirao
Copy link
Author

lirao commented Aug 3, 2023

Hey @lirao, thanks for your contribution. 🙏 Can you please sign the CLA so we can accept your contribution?

Hi, is it ok if someone on the k6 team make this change instead? I can't sign the CLA.

@lirao
Copy link
Author

lirao commented Aug 3, 2023

Closing since I can't sign the CLA. Can someone else make this change instead?

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

Successfully merging this pull request may close these issues.

add metadata to reflection in grpc
4 participants