-
-
Notifications
You must be signed in to change notification settings - Fork 113
Add --ipvx-route-metric option for ha network
#629
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
Conversation
Signed-off-by: David Rapan <[email protected]>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughTwo CLI commands (network update and network vlan) gain new IPv4/IPv6 route metric flags (defaults -1). Flag completion callbacks were added. Argument parsing was extended to accept integer flags and map the route-metric values to the API key. A new exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/network_update.go (1)
159-168: Fix int flag parsing for route metric (currently dropped).
helperIpConfigadds*-route-metric, butparseNetworkArgsonly callsGetStringfor non-array flags. For int flags this returns an error, soroute_metricnever reaches the API. This effectively breaks the new feature.✅ Suggested fix (add int support)
type NetworkArg struct { Arg string ApiKey string IsArray bool + IsInt bool } func parseNetworkArgs(cmd *cobra.Command, args []NetworkArg) map[string]any { networkConfig := make(map[string]any) for _, arg := range args { var val any var err error var changed bool if arg.IsArray { val, err = cmd.Flags().GetStringArray(arg.Arg) changed = len(val.([]string)) > 0 + } else if arg.IsInt { + val, err = cmd.Flags().GetInt(arg.Arg) + changed = true } else { val, err = cmd.Flags().GetString(arg.Arg) changed = val.(string) != "" } if err == nil && changed && cmd.Flags().Changed(arg.Arg) { networkConfig[arg.ApiKey] = val } } return networkConfig } func helperIpConfig(version string, cmd *cobra.Command, options map[string]any) { args := []NetworkArg{ {Arg: version + "-gateway", ApiKey: "gateway"}, {Arg: version + "-method", ApiKey: "method"}, {Arg: version + "-addr-gen-mode", ApiKey: "addr_gen_mode"}, {Arg: version + "-privacy", ApiKey: "ip6_privacy"}, {Arg: version + "-address", ApiKey: "address", IsArray: true}, {Arg: version + "-nameserver", ApiKey: "nameservers", IsArray: true}, - {Arg: version + "-route-metric", ApiKey: "route_metric"}, + {Arg: version + "-route-metric", ApiKey: "route_metric", IsInt: true}, }
Signed-off-by: David Rapan <[email protected]>
Signed-off-by: David Rapan <[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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/network_update.go (1)
145-154: Refactor if-else chain to switch statement per linter.The pipeline failure indicates this if-else chain should be a switch statement. Additionally, setting
changed = trueunconditionally forIsIntis semantically odd (though functionally correct due to theChanged()gate at line 156).♻️ Proposed fix
- if arg.IsInt { - val, err = cmd.Flags().GetInt(arg.Arg) - changed = true - } else if arg.IsArray { - val, err = cmd.Flags().GetStringArray(arg.Arg) - changed = len(val.([]string)) > 0 - } else { - val, err = cmd.Flags().GetString(arg.Arg) - changed = val.(string) != "" + switch { + case arg.IsInt: + val, err = cmd.Flags().GetInt(arg.Arg) + changed = true + case arg.IsArray: + val, err = cmd.Flags().GetStringArray(arg.Arg) + changed = len(val.([]string)) > 0 + default: + val, err = cmd.Flags().GetString(arg.Arg) + changed = val.(string) != "" }
Signed-off-by: David Rapan <[email protected]>
sairon
left a comment
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!
Proposed change
Add options to modify route metric for
ha network update & vlanType of change
Additional information
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.