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

Erroring if gRPC parameters contains headers key #3350

Merged
merged 2 commits into from
Oct 16, 2023

Conversation

olegbespalov
Copy link
Contributor

@olegbespalov olegbespalov commented Sep 22, 2023

What?

In #2371 we deprecated the headers param in favor of metadata.

case "headers":
c.vu.State().Logger.Warn("The headers property is deprecated, replace it with the metadata property, please.")
fallthrough
case "metadata":

Since it seems like a long time passed, we could finally remove the support of the headers.

And as the first step, we could start erroring if the headers param is presented.

Once this PR is merged, I'll open the PR with the far-away milestone to clean the error advising the use of metadata.

Why?

We should clean up the headers

Changelog

Minor breaking change: Previously, gRPC invoke's headers param was deprecated and fell through to the metadata. Since this version of k6, using the headers param will result in an error. Instead, users should use the metadata param.

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)

#3343 (comment)

js/modules/k6/grpc/client.go Outdated Show resolved Hide resolved
@olegbespalov olegbespalov added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Sep 22, 2023
@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: -0.06% ⚠️

Comparison is base (1892f91) 72.94% compared to head (daa77bb) 72.89%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3350      +/-   ##
==========================================
- Coverage   72.94%   72.89%   -0.06%     
==========================================
  Files         261      259       -2     
  Lines       20018    20012       -6     
==========================================
- Hits        14603    14587      -16     
- Misses       4488     4493       +5     
- Partials      927      932       +5     
Flag Coverage Δ
ubuntu 72.89% <0.00%> (-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 82.74% <0.00%> (-0.64%) ⬇️

... and 8 files with indirect coverage changes

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

@codebien codebien added this to the v0.48.0 milestone Oct 4, 2023
@olegbespalov olegbespalov merged commit d1d5969 into master Oct 16, 2023
21 checks passed
@olegbespalov olegbespalov deleted the chore/grpc-headers-param branch October 16, 2023 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes lower prio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants