Add prototool for linting, precommit, Circle, Prow.#685
Merged
istio-testing merged 1 commit intoistio:masterfrom Nov 1, 2018
Merged
Add prototool for linting, precommit, Circle, Prow.#685istio-testing merged 1 commit intoistio:masterfrom
istio-testing merged 1 commit intoistio:masterfrom
Conversation
Contributor
Author
|
@ozevren PTAL |
Contributor
|
How many issues in total? Is there a linter output that shows all issues? |
Contributor
Author
The above ignores are for these issues: Those are the ones that stood out from the crowd, so I added the special cases for them. The other linter rules that were disabled seemed to be consistent across all our protos: Description of those: |
Contributor
|
```
rbac/v1alpha1/rbac.proto:162:3:Field name "roleRef" must be
lower_snake_case.
```
Can you tell when this was added? If this was added after Istio 1.0, we can
try to get it changed before 1.1 ships. We might still be able to change it
as long as the Yaml representation stays the same (which should).
```
mixer/v1/service.proto:46:3:Request type "CheckRequest" should be
defined in the same file as the corresponding service.
mixer/v1/service.proto:46:3:Response type "CheckResponse" should be
defined in the same file as the corresponding service.
mixer/v1/service.proto:51:3:Request type "ReportRequest" should be
defined in the same file as the corresponding service.
mixer/v1/service.proto:51:3:Response type "ReportResponse" should be
defined in the same file as the corresponding service.
```
Seems like a straight up bug.
```
networking/v1alpha3/gateway.proto:306:7:Field name "TLSv1_0" must be
UPPER_SNAKE_CASE.
networking/v1alpha3/gateway.proto:309:7:Field name "TLSv1_1" must be
UPPER_SNAKE_CASE.
networking/v1alpha3/gateway.proto:312:7:Field name "TLSv1_2" must be
UPPER_SNAKE_CASE.
networking/v1alpha3/gateway.proto:315:7:Field name "TLSv1_3" must be
UPPER_SNAKE_CASE.
````
I suspect these have already shipped? Would it be possible to restrict the
ignore scope even further? These files are generally pretty big and I want
to make sure that only the relevant things are ignored.
…On Wed, Oct 31, 2018 at 10:47 AM Jeff Mendoza ***@***.***> wrote:
ignores:
- id: MESSAGE_FIELD_NAMES_LOWER_SNAKE_CASE
files:
- rbac/v1alpha1/rbac.proto
- id: REQUEST_RESPONSE_TYPES_IN_SAME_FILE
files:
- mixer/v1/service.proto
- id: ENUM_FIELD_NAMES_UPPER_SNAKE_CASE
files:
- networking/v1alpha3/gateway.proto
The above ignores are for these issues:
rbac/v1alpha1/rbac.proto:162:3:Field name "roleRef" must be lower_snake_case.
mixer/v1/service.proto:46:3:Request type "CheckRequest" should be defined in the same file as the corresponding service.
mixer/v1/service.proto:46:3:Response type "CheckResponse" should be defined in the same file as the corresponding service.
mixer/v1/service.proto:51:3:Request type "ReportRequest" should be defined in the same file as the corresponding service.
mixer/v1/service.proto:51:3:Response type "ReportResponse" should be defined in the same file as the corresponding service.
networking/v1alpha3/gateway.proto:306:7:Field name "TLSv1_0" must be UPPER_SNAKE_CASE.
networking/v1alpha3/gateway.proto:309:7:Field name "TLSv1_1" must be UPPER_SNAKE_CASE.
networking/v1alpha3/gateway.proto:312:7:Field name "TLSv1_2" must be UPPER_SNAKE_CASE.
networking/v1alpha3/gateway.proto:315:7:Field name "TLSv1_3" must be UPPER_SNAKE_CASE.
Those are the ones that stood out from the crowd, so I added the special
cases for them.
The other linter rules that were disabled seemed to be consistent across
all our protos:
remove:
- FILE_OPTIONS_REQUIRE_JAVA_MULTIPLE_FILES
- FILE_OPTIONS_REQUIRE_JAVA_OUTER_CLASSNAME
- FILE_OPTIONS_REQUIRE_JAVA_PACKAGE
- FILE_OPTIONS_EQUAL_GO_PACKAGE_PB_SUFFIX
- ENUM_FIELD_PREFIXES
- ENUM_ZERO_VALUES_INVALID
Description of those:
FILE_OPTIONS_REQUIRE_JAVA_MULTIPLE_FILES Verifies that the file option "java_multiple_files" is set.
FILE_OPTIONS_REQUIRE_JAVA_OUTER_CLASSNAME Verifies that the file option "java_outer_classname" is set.
FILE_OPTIONS_REQUIRE_JAVA_PACKAGE Verifies that the file option "java_package" is set.
FILE_OPTIONS_EQUAL_GO_PACKAGE_PB_SUFFIX Verifies that the file option "go_package" is equal to $(basename PACKAGE)pb.
ENUM_FIELD_PREFIXES Verifies that all enum fields are prefixed with [NESTED_MESSAGE_NAME_]ENUM_NAME_.
ENUM_ZERO_VALUES_INVALID Verifies that all enum zero value names are [NESTED_MESSAGE_NAME_]ENUM_NAME_INVALID.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#685 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AQR8I84Mg0Q7hQpgrxv_TXOp8dOOcpZxks5uqeItgaJpZM4YEfLz>
.
|
Contributor
|
/lgtm |
Collaborator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ozevren The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
Author
Doesn't look like it, You can just disable rules globally or per-file. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: I want to add FIXMES and Github issues for the files in the
ignores:section of prototool.yaml. If this looks good I'll add those before submitting