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

[v2] Add skip interceptor #364

Merged
merged 6 commits into from Dec 30, 2020
Merged

Conversation

XSAM
Copy link
Contributor

@XSAM XSAM commented Nov 24, 2020

Resolve #363

It's hard to transfer a interceptors.UnaryServerInterceptor into interceptors.Reporter, so I use the old way to implement this feature.

@XSAM XSAM changed the title Add skip interceptor [v2] Add skip interceptor Nov 24, 2020
@codecov-io
Copy link

codecov-io commented Nov 24, 2020

Codecov Report

Merging #364 (6695dad) into v2 (a79558a) will decrease coverage by 7.68%.
The diff coverage is 89.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v2     #364      +/-   ##
==========================================
- Coverage   83.58%   75.90%   -7.69%     
==========================================
  Files          30       31       +1     
  Lines         932      776     -156     
==========================================
- Hits          779      589     -190     
- Misses        114      139      +25     
- Partials       39       48       +9     
Impacted Files Coverage Δ
chain.go 0.00% <ø> (-90.91%) ⬇️
interceptors/logging/payload.go 79.24% <50.00%> (-3.37%) ⬇️
interceptors/retry/retry.go 66.66% <70.00%> (-9.58%) ⬇️
interceptors/skip/interceptor.go 71.42% <71.42%> (ø)
interceptors/logging/interceptors.go 100.00% <100.00%> (ø)
interceptors/logging/logging.go 61.53% <100.00%> (-6.21%) ⬇️
interceptors/ratelimit/ratelimit.go 100.00% <100.00%> (ø)
interceptors/reporter.go 61.53% <100.00%> (-0.97%) ⬇️
interceptors/server.go 85.71% <100.00%> (+0.42%) ⬆️
interceptors/tags/fieldextractor.go 82.75% <100.00%> (-2.54%) ⬇️
... and 31 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 38f54d9...6695dad. Read the comment docs.

Copy link
Collaborator

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice, I like it, but curious why denylist was chosen instead of allowlist approach? 🤔

To me allowlist approach might be more comprehensive. E.g If you want to use logging middleware only on service X. I will add SKIP -> Logging, filter service != x Doable but quite big inversion (:

I think we could consider this, at the end it's flexible enough anyway, just curious what is easier to use.

Skip might be nice for auth aspect, so by default you deny from auth etc.. Maybe does not matter that much 🤗

LGTM, overall 💪

interceptors/skip/examples_test.go Outdated Show resolved Hide resolved
interceptors/skip/examples_test.go Outdated Show resolved Hide resolved
interceptors/skip/interceptor.go Outdated Show resolved Hide resolved
interceptors/skip/interceptor.go Outdated Show resolved Hide resolved
interceptors/skip/interceptor.go Outdated Show resolved Hide resolved
@XSAM
Copy link
Contributor Author

XSAM commented Nov 25, 2020

To me allowlist approach might be more comprehensive.

I agree with that. At the first design, If service == x then skip seems good, but the function is called filter. Therefore, filter service != x seems better.

@XSAM
Copy link
Contributor Author

XSAM commented Dec 2, 2020

@johanbrandhorst Shall we merge it?

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, but could we add this new interceptor to the README too please? Sorry for the delayed review.

@google-cla
Copy link

google-cla bot commented Dec 5, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@XSAM
Copy link
Contributor Author

XSAM commented Dec 5, 2020

@googlebot I consent.

@google-cla
Copy link

google-cla bot commented Dec 5, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@XSAM
Copy link
Contributor Author

XSAM commented Dec 5, 2020

@johanbrandhorst It seems like the CLA bot is not working.

@johanbrandhorst
Copy link
Collaborator

I think @bwplotka might need to consent too.

@XSAM XSAM requested a review from bwplotka December 9, 2020 06:40
@XSAM
Copy link
Contributor Author

XSAM commented Dec 11, 2020

@bwplotka Shall we?

@yashrsharma44
Copy link
Collaborator

yashrsharma44 commented Dec 30, 2020

Yeah, seems like @bwplotka also needs to consent 😛

Copy link
Collaborator

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Sorry for lag, if you think it makes sense I am happy as well. LGTM!

@bwplotka
Copy link
Collaborator

@googlebot I consent.

@google-cla
Copy link

google-cla bot commented Dec 30, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@bwplotka
Copy link
Collaborator

All is good to me, so merging.

@bwplotka bwplotka merged commit 08b17eb into grpc-ecosystem:v2 Dec 30, 2020
@XSAM XSAM deleted the skip-interceptor branch December 31, 2020 00:59
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.

None yet

5 participants