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

ci: check that all code in .proto must have comments; 让 CI工具检查“proto 代码必须包含注释” #623

Closed
seeflood opened this issue Jun 4, 2022 · 11 comments · Fixed by #736
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed kind/ci priority/medium

Comments

@seeflood
Copy link
Member

seeflood commented Jun 4, 2022

What would you like to be added:
We can let CI to check that all code in .proto files must have comments.
For example, we have comments on state API, see https://github.com/mosn/layotto/blob/main/spec/proto/runtime/v1/runtime.proto#L440
image

And these comments are used to generate document, see https://github.com/mosn/layotto/blob/main/docs/en/api_reference/runtime_v1.md#getstaterequest

But unfortunately, we don't have comments on File API. So the corresponding doc is empty. see https://github.com/mosn/layotto/blob/main/docs/en/api_reference/runtime_v1.md#getfilerequestmetadataentry
image

Why is this needed:
Make sure we have necessary comments on api spec

@seeflood seeflood added good first issue Good for newcomers help wanted Extra attention is needed area/ci labels Jun 4, 2022
@seeflood seeflood mentioned this issue Jun 4, 2022
25 tasks
@seeflood seeflood changed the title ci: check that all code in .proto must have comments ci: check that all code in .proto must have comments 让 CI工具检查“proto 代码必须包含注释” Jun 14, 2022
@seeflood seeflood changed the title ci: check that all code in .proto must have comments 让 CI工具检查“proto 代码必须包含注释” ci: check that all code in .proto must have comments; 让 CI工具检查“proto 代码必须包含注释” Jun 21, 2022
@MichaelDeSteven
Copy link
Contributor

A bit of complex for me maybe. I can give it a try.

@seeflood
Copy link
Member Author

seeflood commented Jun 29, 2022

@MichaelDeSteven ok. Thanks!
We need to search for any existing linter to achieve this goal.
I know there are some linters for proto files, but haven't looked into details:
https://github.com/ckaznocha/protoc-gen-lint
https://github.com/uber/prototool
https://github.com/bufbuild/buf

@MichaelDeSteven
Copy link
Contributor

MichaelDeSteven commented Jul 4, 2022

@MichaelDeSteven ok. Thanks! We need to search for any existing linter to achieve this goal. I know there are some linters for proto files, but haven't looked into details: https://github.com/ckaznocha/protoc-gen-lint https://github.com/uber/prototool https://github.com/bufbuild/buf

we can use buf linters to achieve this goal.
here is my step:

  • config buf.yaml in project root directory.
version: v1
lint:
  use:
    - MINIMAL
    - COMMENT_ENUM
    - COMMENT_ENUM_VALUE
    - COMMENT_FIELD
    - COMMENT_MESSAGE
    - COMMENT_ONEOF
    - COMMENT_RPC
    - COMMENT_SERVICE
  • write makefile
    we can execute buf lint to check comment before compile proto files and generate the corresponding doc.

  • update runtime.proto file missing comments

  • update proto file related doc

@seeflood
Copy link
Member Author

seeflood commented Jul 4, 2022

@MichaelDeSteven Sounds great! We can run this linter in our CI
Could u submit a PR for it?

@MichaelDeSteven
Copy link
Contributor

@MichaelDeSteven Sounds great! We can run this linter in our CI Could u submit a PR for it?

sure. I will finish it in this week maybe.

@seeflood
Copy link
Member Author

@MichaelDeSteven Hi, how about you continue with this issue?
As for #669 , I will help do it

@MichaelDeSteven
Copy link
Contributor

MichaelDeSteven commented Jul 22, 2022

@seeflood
Apologies for my delayed response.
ci task has some progress.

  1. buf lint will be executed when we use make proto.doc
    image
    here is result of the above pic.
    empty comment item is more than 100, which will be fixed.

  2. buf lint is not support check trailing comments now but I found the project doc says that trailing comments is support. Maybe we should only support leading comments to simplify issues?

image

Looking forward to your reply.

@seeflood
Copy link
Member Author

seeflood commented Jul 23, 2022

@MichaelDeSteven
Regarding trailing comments: yes, we can only check leading comments to simplify it.

Regarding make proto.doc and buf lint: actually we want to check the comments automatically in the CI, so that when contributors want to modify the proto files (take #556 as an example), the CI can tell him "hey, please write some comments on the .proto files".
Therefore, could you add this buf lint check into the CI ?

Thanks a lot for your contribution!

@MichaelDeSteven
Copy link
Contributor

MichaelDeSteven commented Jul 23, 2022

Hi @seeflood, I hava two question.

  1. The scope of comments. We just need to check the COMMENT_ENUM, right? If we check all of place including COMMENT_ENUM\COMMENT_MESSAGE\COMMENT_SERVICE and so on, I need take some time to add necessary comments.

  2. I have not write CI file experience. The CI of the buf. I just need to follow it, right? But how can I test it? Is there we need to provide makefile that developer can check .proto file in local env before pr?

@seeflood
Copy link
Member Author

seeflood commented Jul 23, 2022

@MichaelDeSteven

  1. I think you can make it check all, including COMMENT_ENUM\COMMENT_MESSAGE\COMMENT_SERVICE.
    You don't have to add comments for all of them yourself, which is tiring.
    We can add comments together in some other PR. Maybe just adding a simple comment "Required" or "Optional" is enough, which indicates whether this field is necessary or not .

  2. Yes you can follow the guide and modify the github workflow file

we need to provide makefile that developer can check .proto file in local env before pr?

Yes, besides github workflow, I think a makefile script is helpful.
But if you find the makefile hard to write, you can split this issue into different PR, one PR for github workflow, another PR for makefile script.

@MichaelDeSteven
Copy link
Contributor

@MichaelDeSteven

  1. I think you can make it check all, including COMMENT_ENUM\COMMENT_MESSAGE\COMMENT_SERVICE.
    You don't have to add comments for all of them yourself, which is tiring.
    We can add comments together in some other PR. Maybe just adding a simple comment "Required" or "Optional" is enough, which indicates whether this field is necessary or not .
  2. Yes you can follow the guide and modify the github workflow file

we need to provide makefile that developer can check .proto file in local env before pr?

Yes, besides github workflow, I think a makefile script is helpful. But if you find the makefile hard to write, you can split this issue into different PR, one PR for github workflow, another PR for makefile script.

I open a pr. Review it plz.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed kind/ci priority/medium
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants