-
Notifications
You must be signed in to change notification settings - Fork 532
use consistent import aliases, enforce importas rule in golanci.yaml #11616
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
base: main
Are you sure you want to change the base?
Conversation
…olanci.yaml Signed-off-by: devesh <[email protected]>
.golangci.yaml
Outdated
- pkg: github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3 | ||
alias: clusterv3 | ||
- pkg: github.com/envoyproxy/go-control-plane/envoy/config/route/v3 | ||
alias: envoy_config_route_v3 |
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 can be routev3
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.
I strongly prefer prefixing the project name to avoid conflicts envoy_config_route_v3|envoyroutev3|envoy_route_v3|envoy_routev3
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.
I'm ok with envoyroutev3 but we need @timflannagan to weigh in lol
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.
No strong opinion as long as we're consistent - envoyroutev3 would be my preference over envoy_route_v3.
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.
Same applies to other envoy types, not just route/v3
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.
Right, I think prefixing the import with envoy sgtm and following the https://go.dev/doc/effective_go#package-names guidance would be my 2c.
.golangci.yaml
Outdated
@@ -62,6 +62,18 @@ linters: | |||
alias: metav1 | |||
- pkg: k8s.io/api/batch/v1 | |||
alias: batchv1 | |||
- pkg: github.com/envoyproxy/go-control-plane/envoy/config/endpoint/v3 | |||
alias: envoy_config_endpoint_v3 |
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 can be endpointv3
.golangci.yaml
Outdated
- pkg: github.com/envoyproxy/go-control-plane/pkg/cache/types | ||
alias: envoycachetypes | ||
- pkg: github.com/envoyproxy/go-control-plane/envoy/extensions/transport_sockets/tls/v3 | ||
alias: envoyauth |
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.
perhaps this could be tlsv3
@@ -141,9 +141,9 @@ func (h *httpRouteConfigurationTranslator) computeVirtualHost( | |||
|
|||
type backendConfigContext struct { | |||
typedPerFilterConfigRoute ir.TypedFilterConfigMap | |||
RequestHeadersToAdd []*envoy_config_core_v3.HeaderValueOption | |||
RequestHeadersToAdd []*corev3.HeaderValueOption |
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.
I think that this makes us lose a lot of context.
what is core with regards to our routes?
I am not convinced on this approach see #11607 (comment) |
Thank you, maintainers, for the review. I will now update the PR with the suggested changes: envoyroutev3, envoyclusterv3, envoycorev3, tlsv3, and envoyendpointv3. |
@devc007 make sure to use "envoy" in the name for all, so make it |
…ter context Signed-off-by: devesh <[email protected]>
Signed-off-by: Devesh chauhan <[email protected]>
Description
Fixes #11607
Change Type
Changelog