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

fix(grpc-transcode): filter out illegal INT(string) formats #11367

Merged
merged 3 commits into from
Jul 9, 2024
Merged

fix(grpc-transcode): filter out illegal INT(string) formats #11367

merged 3 commits into from
Jul 9, 2024

Conversation

zhoujiexiong
Copy link
Contributor

@zhoujiexiong zhoujiexiong commented Jun 21, 2024 •
edited
Loading

Description

Need to sync. lua-protobuf lib. updated by this PR(starwing/lua-protobuf#269).

Fixes #11355

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@zhoujiexiong
Copy link
Contributor Author

Hi @yzeng25 ,
It seems that other test case unrelated fail the check, please help confirm.

image

@zhoujiexiong
Copy link
Contributor Author

zhoujiexiong commented Jun 24, 2024 •
edited
Loading

Hi @yzeng25 , It seems that other test case unrelated fail the check, please help confirm.

image

Hi @yzeng25 ,
Still like that, unrelated.
image

@yzeng25
Copy link
Contributor

yzeng25 commented Jun 27, 2024

Hi @yzeng25 , It seems that other test case unrelated fail the check, please help confirm.
image

Hi @yzeng25 , Still like that, unrelated. image

Let's seek help from others who are more familiar with CIs. @pottekkat @shreemaan-abhishek @nic-6443

@zhoujiexiong
Copy link
Contributor Author

Hi @yzeng25 , It seems that other test case unrelated fail the check, please help confirm.
image

Hi @yzeng25 , Still like that, unrelated. image

Let's seek help from others who are more familiar with CIs. @pottekkat @shreemaan-abhishek @nic-6443

Checks had passed, reviewers are needed. :-)
image

nic-6443
nic-6443 previously approved these changes Jul 2, 2024
View reviewed changes
shreemaan-abhishek
shreemaan-abhishek reviewed Jul 3, 2024
View reviewed changes
t/plugin/grpc-transcode3.t Outdated Show resolved Hide resolved
t/plugin/grpc-transcode3.t Outdated
--pb.option "int64_as_string"
--pb.option "int64_as_hexstring"
pb_encode("IntStringPatterns", supported)
local status, err = pcall(pb_encode, "IntStringPatterns", unsupported)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to avoid using repeated and check all supported/unsupported types in a loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shreemaan-abhishek test case updated, pls. re-check.

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you, I will check this PR after the CI completes run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The check errors are basically the same.

image

@zhoujiexiong
Copy link
Contributor Author

zhoujiexiong commented Jul 3, 2024 •
edited
Loading

@shreemaan-abhishek @nic-6443

Hi Pals,
One more reviewer needed, could you help?

Ping @yzeng25 :D

@yzeng25 yzeng25 requested a review from nic-6443 July 4, 2024 01:47
@moonming moonming merged commit 1164374 into apache:master Jul 9, 2024
39 of 48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shreemaan-abhishek shreemaan-abhishek shreemaan-abhishek approved these changes

@moonming moonming moonming approved these changes

@nic-6443 nic-6443 nic-6443 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@zhoujiexiong @yzeng25 @nic-6443 @moonming @shreemaan-abhishek