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.

Sign up for GitHub

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

Jump to bottom

Move util/metautils to root-level package metadata, fixes #392 #474

Merged
merged 2 commits into from
Dec 7, 2021
Merged

Move util/metautils to root-level package metadata, fixes #392 #474

merged 2 commits into from
Dec 7, 2021

Conversation

rahulkhairwar
Copy link
Contributor

@rahulkhairwar rahulkhairwar commented Nov 22, 2021 •
edited by johanbrandhorst

  • Move util/metautils to root-level package metadata
  • Rename NiceMD to MD, which is a wrapper for grpc/metadata.MD

Fixes #392

@rahulkhairwar rahulkhairwar mentioned this pull request Nov 22, 2021
Closed
@rahulkhairwar rahulkhairwar changed the title #392 Move util/metautils to root-level package metadata #392 Nov 22, 2021
@rahulkhairwar rahulkhairwar changed the title Move util/metautils to root-level package metadata #392 Move util/metautils to root-level package metadata Nov 30, 2021
@rahulkhairwar rahulkhairwar force-pushed the metadata-package-issue392 branch 2 times, most recently from d53c0dc to 72e66ee Compare December 1, 2021 12:36
@rahulkhairwar rahulkhairwar changed the title Move util/metautils to root-level package metadata Move util/metautils to root-level package metadata, fixes #392 Dec 2, 2021
@johanbrandhorst
Copy link
Collaborator

Hi Rahul, thanks for your PR. Have you signed the CLA at https://cla.developers.google.com/clas?

@codecov-commenter
Copy link

codecov-commenter commented Dec 3, 2021 •
edited

Codecov Report

Merging #474 (fd819cb) into v2 (6e2c2ac) will decrease coverage by 26.43%.
The diff coverage is 48.79%.

Impacted file tree graph

@@             Coverage Diff             @@
##               v2     #474       +/-   ##
===========================================
- Coverage   84.01%   57.58%   -26.44%     
===========================================
  Files          30       28        -2     
  Lines         932     1530      +598     
===========================================
+ Hits          783      881       +98     
- Misses        110      586      +476     
- Partials       39       63       +24
Impacted Files Coverage Δ
chain.go 0.00% <ø> (-90.91%) ⬇️
interceptors/auth/auth.go 100.00% <ø> (ø)
interceptors/ratelimit/ratelimit.go 60.00% <0.00%> (-40.00%) ⬇️
interceptors/recovery/options.go 78.57% <ø> (ø)
metadata/single_key.go 60.00% <ø> (ø)
testing/testpb/interceptor_suite.go 0.00% <0.00%> (ø)
testing/testpb/test.manual_validator.pb.go 0.00% <0.00%> (ø)
util/backoffutils/backoff.go 60.00% <ø> (ø)
wrappers.go 66.66% <ø> (-33.34%) ⬇️
interceptors/reporter.go 45.45% <20.00%> (-17.05%) ⬇️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd1540e...fd819cb. Read the comment docs.

@rahulkhairwar
Copy link
Contributor Author

hi @johanbrandhorst , sorry I missed it earlier. I've signed it now.

johanbrandhorst
johanbrandhorst requested changes Dec 4, 2021
View reviewed changes
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Hi, the CLA bot still doesn't seem to have recognized your signature, could you please share a screenshot of the signed CLA from the website? Thanks

testing/testpb/v1/test.proto Outdated
@@ -2,7 +2,7 @@ syntax = "proto3";

package testing.testpb.v1;

option go_package = ".;testpb";
option go_package = "./;testpb";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this change made?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't run make all without this change, see below:
Screenshot 2021-12-05 at 11 57 35 PM

I guess different versions of protoc-gen.
Let me revert this change and push again.

@rahulkhairwar
Fix grpc-ecosystem#392:
3267a84 
- Move util/metautils to root-level package metadata
- Rename NiceMD to MD, which is a wrapper for grpc/metadata.MD
@rahulkhairwar
Copy link
Contributor Author

Also, for the CLA, please look at the screenshot below:
Screenshot 2021-12-06 at 12 00 50 AM

johanbrandhorst
johanbrandhorst requested changes Dec 7, 2021
View reviewed changes
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Thanks for signing the CLA. There are still some outstanding issues here.

interceptors/auth/auth_test.go Outdated Show resolved Hide resolved
interceptors/auth/metadata.go Outdated Show resolved Hide resolved
interceptors/auth/metadata_test.go Outdated Show resolved Hide resolved
interceptors/retry/retry.go Outdated Show resolved Hide resolved
interceptors/skip/interceptor_test.go Show resolved Hide resolved
testing/testpb/test_grpc.pb.go Outdated Show resolved Hide resolved
testing/testpb/test.pb.go Outdated Show resolved Hide resolved
johanbrandhorst
johanbrandhorst approved these changes Dec 7, 2021
View reviewed changes
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

LGTM!

@johanbrandhorst johanbrandhorst merged commit a5b9e0b into grpc-ecosystem:v2 Dec 7, 2021
@johanbrandhorst
Copy link
Collaborator

Thank you for your contribution!

@rahulkhairwar rahulkhairwar deleted the metadata-package-issue392 branch December 8, 2021 04:16
@aimuz aimuz mentioned this pull request Mar 8, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanbrandhorst johanbrandhorst johanbrandhorst approved these changes

@bwplotka bwplotka Awaiting requested review from bwplotka

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rahulkhairwar @johanbrandhorst @codecov-commenter