Make maxports a global flag, add flag help #203
Conversation
Summary of ChangesHello @hansr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant improvements to the command-line interface by centralizing the 'maxports' configuration and enhancing the help system. The 'maxports' flag is now global, defaulting to an unlimited setting to maintain backward compatibility. Additionally, users can now request specific help for any flag, providing more granular and accessible documentation directly from the command line. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the maxports flag to be a global flag instead of being defined on multiple subcommands. This is a good simplification. The PR also introduces a helpful feature to display help for individual flags. My review includes a suggestion to improve the implementation of the flag help feature for better maintainability. I've also pointed out a change in the default behavior for the maxports flag on some commands, which now defaults to unlimited ports instead of 10, which could be an unexpected change for some users.
| // DEPRECATED: maxports is now a global flag - use 'diode -maxports=<value> gateway' instead | ||
| gatewayCmd.Flag.IntVar(&cfg.MaxPortsPerDevice, "maxports", 0, "DEPRECATED: use global -maxports flag instead (maximum concurrent ports per device, 0 = unlimited)") |
There was a problem hiding this comment.
The default value for maxports has been changed from 10 to 0 (unlimited). This is a behavioral change for users who were not explicitly setting this flag and relying on the default limit. While making maxports global is a good improvement, this change in default could have unintended consequences, such as resource exhaustion, if users were implicitly depending on the previous limit of 10. It might be safer to keep the default at 10 for the deprecated command-specific flags to maintain backward compatibility, while the new global flag defaults to 0. This comment also applies to publish.go and socksd.go where the same change was made.
| func isBoolValue(f *flag.Flag) bool { | ||
| typ := reflect.TypeOf(f.Value) | ||
| if typ.Kind() != reflect.Ptr { | ||
| return false | ||
| } | ||
| return typ.Elem().String() == "flag.boolValue" | ||
| } |
There was a problem hiding this comment.
The flag package provides an exported method IsBoolFlag() on *flag.Flag to check if a flag is a boolean. Using this method is more robust and idiomatic than relying on reflection to inspect unexported type names, which can be brittle and break with future Go versions.
func isBoolValue(f *flag.Flag) bool {
return f.IsBoolFlag()
}
This PR makes the "maxports" a global flag, and doesn't break if someone has already started defining it on a specific command.
It also sets default to "0" (unlimited) so previous usages that were effectively using unlimited connections won't be impacted.
It also adds support for flag help. e.g. diode -maxports --help will now show help output.