-
Notifications
You must be signed in to change notification settings - Fork 301
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
feat: add namespaced all proxies config dump support #1082
Conversation
cc @zirain |
Codecov Report
@@ Coverage Diff @@
## main #1082 +/- ##
==========================================
- Coverage 61.49% 61.39% -0.10%
==========================================
Files 79 79
Lines 11439 11465 +26
==========================================
+ Hits 7034 7039 +5
- Misses 3944 3966 +22
+ Partials 461 460 -1
|
cc @arkodg, please take a look too. |
Signed-off-by: bitliu <bitliu@tencent.com>
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.
🚀 ❤️
@@ -113,10 +113,28 @@ func retrieveConfigDump(args []string, includeEds bool, configType envoyConfigTy | |||
// fetchRunningEnvoyPods gets the Pods, either based on the NamespacedName or the labelSelectors. | |||
// It further filters out only those Pods that are in "Running" state. | |||
// labelSelectors, if provided, take precedence over the pod NamespacedName. | |||
func fetchRunningEnvoyPods(c kube.CLIClient, nn types.NamespacedName, labelSelectors []string) ([]types.NamespacedName, error) { | |||
func fetchRunningEnvoyPods(c kube.CLIClient, nn types.NamespacedName, labelSelectors []string, allNamespaces bool) ([]types.NamespacedName, error) { |
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.
IMO nn
is not readable , prefer name
or namespacedName
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 did not change the previous vars, I think this is nit and not a blocker for this PR.
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
some nit
xref: #1069