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

[interceptors/validator] feat: add error logging in validator #544

Merged
merged 8 commits into from Mar 26, 2023

Conversation

rohanraj7316
Copy link
Contributor

used logging.Logger interface to
add error logging in validator
interceptor

addition: #494

@rohanraj7316 rohanraj7316 changed the title feat: add error logging in validator [interceptors/validator] feat: add error logging in validator Mar 21, 2023
@bwplotka bwplotka added the v2 label Mar 22, 2023
used logging.Logger interface to
add error logging in validator
interceptor

addition: grpc-ecosystem#494
made fast fail and logger as optional args
addition to that instead of providing values
dynamically at the time of initialization made
it more dynamic
@rohanraj7316
Copy link
Contributor Author

rohanraj7316 commented Mar 23, 2023

@bwplotka have updated the implementation do let me your feedback. if it looks good🤞🤞 then I will update the doc and test cases too. just wanted to be sure about the implementation first 🙈. I have taken inspiration from logging module

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.

Amazing work! Generally great direction, just some nits.

interceptors/validator/options.go Outdated Show resolved Hide resolved
interceptors/validator/options.go Outdated Show resolved Hide resolved
interceptors/validator/validator.go Show resolved Hide resolved
updated args based on review
restructured if statement  in-order to make code execution based on shouldFailFast flag more relevant.
restructured code in order to separate the
concern. ie: in terms of code struct and testcases wise.
modified testcases based on the current modifications made in the code base.
@rohanraj7316
Copy link
Contributor Author

@bwplotka can you help me to figure out what's the lint issue?

@bwplotka
Copy link
Collaborator

Just run "make lint" locally and commit changes (some automatic fixes got applied)

interceptors/validator/options.go Outdated Show resolved Hide resolved
interceptors/validator/options.go Show resolved Hide resolved
@bwplotka
Copy link
Collaborator

make lint again please until it does not show any more errors locally (:

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.

LGTM! Just lint. Thanks!

I can also just merge and fix lint on my own, that's fine - I plan to do one more thing.

@rohanraj7316
Copy link
Contributor Author

rohanraj7316 commented Mar 25, 2023

sure go ahead actually, I tried running lint at my end but it's missing the initialization of GOIMPORTS in env. thought lint shows fine when tried adding goimports -w $1 as mentioned on goimports.sh. so still figuring out the actual cause 🥺

@bwplotka
Copy link
Collaborator

What is an error? Because GOIMPORTS is defined in Makefile variable file in .bingo/Variables.mk

Anyway, merging, thanks!

@bwplotka bwplotka merged commit e41e6bd into grpc-ecosystem:v2 Mar 26, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants