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

feat: support proto API #2099

Merged
merged 21 commits into from
Oct 19, 2021
Merged

feat: support proto API #2099

merged 21 commits into from
Oct 19, 2021

Conversation

bzp2010
Copy link
Contributor

@bzp2010 bzp2010 commented Aug 26, 2021

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What changes will this PR take into?

Add proto entity support to Manager API.

Related issues

resolve part of #1378 #1449

Checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@bzp2010 bzp2010 added this to the 2.7.2 milestone Aug 26, 2021
@bzp2010 bzp2010 self-assigned this Aug 26, 2021
@bzp2010 bzp2010 force-pushed the feat-proto-api branch 2 times, most recently from 44eea86 to 781fa52 Compare August 27, 2021 07:08
@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2021

Codecov Report

Merging #2099 (a2bc7a8) into master (a8298f4) will decrease coverage by 18.95%.
The diff coverage is 0.00%.

❗ Current head a2bc7a8 differs from pull request most recent head 5539b2d. Consider uploading reports for the commit 5539b2d to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2099       +/-   ##
===========================================
- Coverage   69.34%   50.39%   -18.96%     
===========================================
  Files         188       40      -148     
  Lines        7161     3072     -4089     
  Branches      823        0      -823     
===========================================
- Hits         4966     1548     -3418     
+ Misses       1906     1330      -576     
+ Partials      289      194       -95     
Flag Coverage Δ
backend-e2e-test ?
backend-e2e-test-ginkgo ?
backend-unit-test 50.39% <0.00%> (-1.93%) ⬇️
frontend-e2e-test ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/internal/handler/proto/proto.go 0.00% <0.00%> (ø)
api/main.go 0.00% <0.00%> (-100.00%) ⬇️
api/internal/utils/version.go 0.00% <0.00%> (-100.00%) ⬇️
api/internal/filter/request_id.go 0.00% <0.00%> (-100.00%) ⬇️
api/internal/core/entity/entity.go 0.00% <0.00%> (-90.91%) ⬇️
api/internal/filter/cors.go 0.00% <0.00%> (-70.00%) ⬇️
api/internal/core/store/storehub.go 0.00% <0.00%> (-68.80%) ⬇️
api/internal/utils/closer.go 0.00% <0.00%> (-66.67%) ⬇️
api/internal/filter/schema.go 0.00% <0.00%> (-56.00%) ⬇️
api/internal/utils/consts/api_error.go 0.00% <0.00%> (-50.00%) ⬇️
... and 172 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 a8298f4...5539b2d. Read the comment docs.

@liuxiran liuxiran modified the milestones: 2.7.2, 2.9 Aug 31, 2021
@bzp2010 bzp2010 changed the title feat: support proto api feat: support proto API Sep 18, 2021
@bzp2010
Copy link
Contributor Author

bzp2010 commented Sep 18, 2021

Do you have any suggestions on compiling and verifying the correctness of protobuf syntax in real-time?
I haven't found a way to integrate this function into the program, most of which is the use of CLI.

@bzp2010 bzp2010 force-pushed the feat-proto-api branch 8 times, most recently from fe4339f to d9a69be Compare September 18, 2021 14:47
@bzp2010 bzp2010 marked this pull request as ready for review September 18, 2021 15:32
@bzp2010
Copy link
Contributor Author

bzp2010 commented Sep 18, 2021

Hi, everyone. This is some information about the current PR.

Complete status:

  • functional code writing
  • unit test
  • E2E test
  • API Document

Current limitations:

Now I haven't found a way to integrate the protobuf golang generator into the program, so I can't verify the protobuf entered by the user in real time and point out the error.
I noticed that software such as protoc-gen-go and protoc-gen-gogo are written as protoc plugins, completely using stdin and stdout to interact with protoc, and there is no need to consider the possibility of introducing other programs as dependency libraries.

Need help:

I wonder if you have a better way. If so, please point it out and I will continue to improve it.

@juzhiyuan
Copy link
Member

Hi @ShiningRush @shuaijinchao, any ideas about this?

Copy link
Member

@nic-chen nic-chen left a comment

Choose a reason for hiding this comment

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

the PR is too big. We could split it into several small PRs.

@bzp2010
Copy link
Contributor Author

bzp2010 commented Sep 19, 2021

the PR is too big. We could split it into several small PRs.

Yes, I think so. I will try it.

BTW, should I split to function, unit test, e2e test, that 3 PRs?

@nic-chen
Copy link
Member

the PR is too big. We could split it into several small PRs.

Yes, I think so. I will try it.

BTW, should I split to function, unit test, e2e test, that 3 PRs?

We could split it to: structure and schema definition, create...
And each PR has corresponding complete test cases.
That would be better.

@bzp2010 bzp2010 force-pushed the feat-proto-api branch 2 times, most recently from adaa0bd to 3dee8dd Compare October 14, 2021 03:00
@bzp2010
Copy link
Contributor Author

bzp2010 commented Oct 19, 2021

ping @nic-chen

@bzp2010 bzp2010 merged commit 9f17637 into apache:master Oct 19, 2021
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.

6 participants