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

Adding support for overriding grpc authority pseudo header #3454

Closed
wants to merge 1 commit into from

Conversation

ali-atac
Copy link

@ali-atac ali-atac commented Nov 14, 2023

What?

Why?

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make 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)

@CLAassistant
Copy link

CLAassistant commented Nov 14, 2023

CLA assistant check
All committers have signed the CLA.

@ali-atac ali-atac marked this pull request as draft November 14, 2023 14:46
Copy link
Contributor

@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 @ali-atac,
it seems a reasonable change as we already discussed in the past.
I see this pull request is in a draft state. However, as it hasn't changed for some days, let me clarify and remind you that signing the CLA is required to be able to merge your contribution.
I encourage you also to discuss before in a dedicated issue your proposal before spending relevant effort on it.

@olegbespalov olegbespalov added the awaiting user waiting for user to respond label Nov 23, 2023
@ali-atac ali-atac marked this pull request as ready for review November 27, 2023 15:25
@codecov-commenter
Copy link

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (24ae4d9) 73.35% compared to head (96156bb) 73.28%.
Report is 5 commits behind head on master.

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

Files Patch % Lines
js/modules/k6/grpc/client.go 12.50% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3454      +/-   ##
==========================================
- Coverage   73.35%   73.28%   -0.08%     
==========================================
  Files         266      264       -2     
  Lines       19965    19968       +3     
==========================================
- Hits        14646    14633      -13     
- Misses       4409     4420      +11     
- Partials      910      915       +5     
Flag Coverage Δ
ubuntu 73.28% <12.50%> (-0.03%) ⬇️
windows ?

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@olegbespalov olegbespalov removed the awaiting user waiting for user to respond label Dec 5, 2023
@mstoykov
Copy link
Contributor

mstoykov commented Dec 7, 2023

I would really prefer if we get a test about this.

Also this likely will conflict with #3152

@ali-atac
Copy link
Author

ali-atac commented Dec 7, 2023

Thanks for the comment, I'll add some tests next week

Copy link
Contributor

@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.

Hi @ali-atac,
#3152 has been merged, so you can now rebase to finalize the code. Are you still interested in working on it?

@codebien
Copy link
Contributor

Hey!
I close this pull request because the offical doc mentions that authority is already a supported case for the dns:// resolver (the syntax is dns:[//authority/]host[:port]). If someone hits an issue with it, a specific bug report or a unit test is necessary before merging any dedicated logic for it.

@codebien codebien closed this Feb 28, 2024
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.

6 participants