added a cluster allowlist param for vs based routing #346
Conversation
Signed-off-by: Shriram Sharma <[email protected]>
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #346 +/- ##
==========================================
+ Coverage 71.76% 71.90% +0.13%
==========================================
Files 66 66
Lines 9355 9365 +10
==========================================
+ Hits 6714 6734 +20
+ Misses 2279 2266 -13
- Partials 362 365 +3 ☔ View full report in Codecov by Sentry. |
| if !wrapper.params.EnableVSRouting { | ||
| return false | ||
| } | ||
| for _, c := range wrapper.params.VSRoutingEnabledClusters { |
There was a problem hiding this comment.
If EnableVSRouting is true and VSRoutingEnabledClusters is "[]", does that mean no clusters should get the VS?
What is the correct way to enable it across the board without adding clusters to the list?
There was a problem hiding this comment.
@adilfulara , I'll be adding a check for *. If that exists in the cluster list, then it will be enabled for all clusters.
| @@ -254,6 +254,7 @@ func GetRootCmd(args []string) *cobra.Command { | |||
|
|
|||
| // Flags pertaining to VS based routing | |||
| rootCmd.PersistentFlags().BoolVar(¶ms.EnableVSRouting, "enable_vs_routing", false, "Enable/Disable VS Based Routing") | |||
There was a problem hiding this comment.
This probably needs to be a string with enabled/disabled/all or something
There was a problem hiding this comment.
@vinay-g would this work? #346 (comment)
| continue | ||
| } | ||
|
|
||
| if remoteRegistry == nil { |
There was a problem hiding this comment.
Can this check be done only once ?
Signed-off-by: Shriram Sharma <[email protected]>
| { | ||
| name: "Given enableVSRouting is true, and all VS routing is enabled in all clusters using '*'" + | ||
| "When DoVSRoutingForCluster is called" + | ||
| "Then it should return false", |
There was a problem hiding this comment.
Then it should return true :)
Description
What does this change do and why?
Added a cluster allow list to specify on which source clusters VS based routing should be enabled.