-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
protos: Update style guide to try and prevent redundant imports #16231
Conversation
Signed-off-by: Ryan Northey <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Why not verify this works as expected on some extension?
@@ -122,8 +122,7 @@ To add an extension config to the API, the steps below should be followed: | |||
deps = ["@com_github_cncf_udpa//udpa/annotations:pkg"], | |||
) | |||
``` | |||
1. Add to the v3 extension config proto `import "udpa/annotations/migrate.proto";` | |||
and `import "udpa/annotations/status.proto";` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im guessing in this case that its ok to just totally remove this
@@ -122,8 +122,7 @@ To add an extension config to the API, the steps below should be followed: | |||
deps = ["@com_github_cncf_udpa//udpa/annotations:pkg"], | |||
) | |||
``` | |||
1. Add to the v3 extension config proto `import "udpa/annotations/migrate.proto";` | |||
and `import "udpa/annotations/status.proto";` | |||
1. Add to the v3 extension config proto `import "udpa/annotations/status.proto";` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about this one - this is also added and unused frequently - but im guessing it is needed (in some cases ?) and may relate to some of the docs below ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed for the work_in_progress
annotation below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k - on that basis lets keep - i think the issue is more status not being set than the status proto being included
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
…yproxy#16231) Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: Gokul Nair <[email protected]>
…yproxy#16231) Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: Gokul Nair <[email protected]>
Signed-off-by: Ryan Northey [email protected]
Commit Message: protos: Update style guide to try and prevent redundant imports
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] Partial fix for #15589
[Optional Deprecated:]
[Optional API Considerations:]