Clean up comments on iface.go #12844
Conversation
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
github-actions
bot
added
kind/cleanup
There was a problem hiding this comment.
Pull Request Overview
This PR improves code quality through grammar corrections and documentation enhancements. The primary focus is correcting the grammatically incorrect phrase "lesser than" to the proper "less than" in validation error messages across the codebase.
Key Changes:
- Fixed grammar in validation error messages from "lesser than" to "less than"
- Enhanced documentation comments in the ProxyTranslationPass interface
- Reorganized the order of methods in the ProxyTranslationPass interface for better logical flow
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/e2e/tests/api_validation_test.go | Updated test case names and expected error messages to use correct grammar |
| api/v1alpha1/traffic_policy_types.go | Corrected validation error message in kubebuilder annotation |
| install/helm/kgateway-crds/templates/gateway.kgateway.dev_trafficpolicies.yaml | Updated CRD validation message to match API changes |
| pkg/pluginsdk/ir/iface.go | Improved interface documentation with clearer comments and reordered ApplyRouteConfigPlugin method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
|
|
||
| // ApplyRouteConfigPlugin is called 1 time for all the routes in a filter chain. Use this to set default PerFilterConfig | ||
| // Applies policy for a Gateway that has a policy attached via a targetRef. |
There was a problem hiding this comment.
Applies policy for a Gateway that has a policy attached via a targetRef.
Is this fully correct? I think it is only called in certain circumstances, e.g. only for HTTP (or HTTPS) listeners? (as opposed to ApplyVHost)
There was a problem hiding this comment.
I'm not entirely sure. I think ApplyForVHost runs when section name is used, where ApplyRouteConfigPlugin runs when targeting whole gateway
There was a problem hiding this comment.
feel free to check my work but this is what I noted in the code
- ComputeListener
- ApplyListenerPlugin
- ComputeRouteConfiguration
- computeVirtualHost
- envoyRoutes
- translateRouteAction
- ApplyForBackend
- ApplyForRouteBackend
- runRoutePlugins
- ApplyForRoute
- runVhostPlugins
- ApplyVhostPlugin
- ApplyRouteConfigPlugin
- HttpFilters
2 participants
Description
The comment says
Each of the functions here will be called in the order they appear in the interface.This was not true.Change Type
/kind cleanup
Changelog
Additional Notes