refactor: options package returns errors instead of os.Exit#443
refactor: options package returns errors instead of os.Exit#443
Conversation
- Changed Parse() to return (*Opts, error) instead of calling os.Exit - Created ConfigError type that collects multiple validation errors - Updated envIntOrDefault and envBoolOrDefault to accumulate errors - Added deprecated MustParse() for backward compatibility - Updated main.go to handle Parse() errors properly - Use custom FlagSet to allow multiple Parse() calls in tests - Fixed race condition in resettoken package by adding mutex protection for defaultClock global variable - Added comprehensive tests for error handling paths - Options package coverage improved from ~0% to 85.4%
Summary of ChangesHello @CybotTM, 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 refactors the application's options parsing mechanism to adhere to more robust and idiomatic Go error handling principles. By transitioning from immediate program termination ( 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. Changelog
Activity
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
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #443 +/- ##
===========================================
+ Coverage 74.71% 92.41% +17.69%
===========================================
Files 15 15
Lines 704 725 +21
===========================================
+ Hits 526 670 +144
+ Misses 167 46 -121
+ Partials 11 9 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
This PR refactors configuration parsing to improve error handling and testability. The main change replaces immediate exit-on-failure behavior with error accumulation, allowing all configuration issues to be reported simultaneously through a new ConfigError type. The Parse() function now returns errors instead of calling os.Exit(), with a deprecated MustParse() wrapper preserving backward compatibility. Thread-safety improvements were added to the clock implementation in the reset token module using read-write mutexes to prevent race conditions during concurrent access. The main function was updated to handle configuration errors gracefully with proper logging. Comprehensive test coverage was added for all new error handling paths.
Changes
| File(s) | Summary |
|---|---|
internal/options/app.go |
Refactored configuration parsing to accumulate errors instead of immediate exit. Introduced ConfigError type for collecting multiple validation errors. Modified Parse() to return (*Opts, error) and updated helper functions to accept *ConfigError parameter. Added deprecated MustParse() wrapper for backward compatibility. Replaced os.Exit(1) calls with error collection and return. |
internal/options/app_test.go |
Enhanced test coverage with error handling validation for environment variable parsing. Added test cases for invalid inputs (non-numeric, negative, overflow, invalid booleans). Introduced integration tests for missing required fields and invalid values. Added TestConfigError to validate error accumulation and formatting. Updated existing tests to verify ConfigError state. |
internal/resettoken/clock.go |
Added thread-safety with sync.RWMutex to protect defaultClock variable. Implemented getClock() helper function with read-locking for safe concurrent access. |
internal/resettoken/clock_test.go |
Added mutex locking to setTestClock helper function and its cleanup logic to prevent race conditions during concurrent test clock operations. |
internal/resettoken/store.go |
Refactored IsExpired() method to use getClock() function instead of directly accessing defaultClock for improved testability. |
main.go |
Enhanced error handling for configuration parsing with explicit error checking, logging via slog.Error(), and exit with status code 1 on failure. Moved err variable declaration inline with Parse() call. |
Sequence Diagram
This diagram shows the interactions between components:
sequenceDiagram
participant Caller
participant Parse
participant ConfigError
participant FlagSet
participant EnvHelpers as envIntOrDefault/envBoolOrDefault
participant Validators as checkRequired
participant MustParse
Note over Caller,MustParse: Configuration Parsing Flow
alt Direct Parse() call
Caller->>Parse: Parse()
activate Parse
Parse->>Parse: Load .env files
Parse->>ConfigError: Create new ConfigError{}
Parse->>FlagSet: NewFlagSet("gopherpass")
loop For each configuration flag
Parse->>EnvHelpers: envStringOrDefault(name, default)
EnvHelpers-->>Parse: string value
Parse->>EnvHelpers: envIntOrDefault(name, default, errs)
alt Invalid integer format
EnvHelpers->>ConfigError: Add(error message)
EnvHelpers-->>Parse: return default value
else Valid integer
EnvHelpers-->>Parse: return parsed value
end
Parse->>EnvHelpers: envBoolOrDefault(name, default, errs)
alt Invalid boolean format
EnvHelpers->>ConfigError: Add(error message)
EnvHelpers-->>Parse: return default value
else Valid boolean
EnvHelpers-->>Parse: return parsed value
end
end
Parse->>FlagSet: Parse(os.Args[1:])
alt Flag parsing error
FlagSet-->>Parse: error
Parse->>ConfigError: Add(flag parsing error)
end
loop For each required field
Parse->>Validators: checkRequired(name, value, missing)
Validators-->>Parse: updates missing list
end
alt Missing required fields
Parse->>ConfigError: Add(missing options message)
end
Parse->>ConfigError: HasErrors()
alt Has errors
ConfigError-->>Parse: true
Parse-->>Caller: nil, ConfigError
else No errors
ConfigError-->>Parse: false
Parse->>Parse: Create Opts struct
Parse-->>Caller: *Opts, nil
end
deactivate Parse
else MustParse() call (backward compatibility)
Caller->>MustParse: MustParse()
activate MustParse
MustParse->>Parse: Parse()
Parse-->>MustParse: *Opts, error
alt Error occurred
MustParse->>MustParse: fmt.Fprintf(stderr, error)
MustParse->>MustParse: os.Exit(1)
else Success
MustParse-->>Caller: *Opts
end
deactivate MustParse
end
🔗 Cross-Repository Impact Analysis
Enable automatic detection of breaking changes across your dependent repositories. → Set up now
Learn more about Cross-Repository Analysis
What It Does
- Automatically identifies repositories that depend on this code
- Analyzes potential breaking changes across your entire codebase
- Provides risk assessment before merging to prevent cross-repo issues
How to Enable
- Visit Settings → Code Management
- Configure repository dependencies
- Future PRs will automatically include cross-repo impact analysis!
Benefits
- 🛡️ Prevent breaking changes across repositories
- 🔍 Catch integration issues before they reach production
- 📊 Better visibility into your multi-repo architecture
Install the extension
Note for Windsurf
Please change the default marketplace provider to the following in the windsurf settings:Marketplace Extension Gallery Service URL: https://marketplace.visualstudio.com/_apis/public/gallery
Marketplace Gallery Item URL: https://marketplace.visualstudio.com/items
Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts below
Emoji Descriptions:
⚠️ Potential Issue - May require further investigation.- 🔒 Security Vulnerability - Fix to ensure system safety.
- 💻 Code Improvement - Suggestions to enhance code quality.
- 🔨 Refactor Suggestion - Recommendations for restructuring code.
- ℹ️ Others - General comments and information.
Interact with the Bot:
- Send a message or request using the format:
@entelligenceai + *your message*
Example: @entelligenceai Can you suggest improvements for this code?
- Help the Bot learn by providing feedback on its responses.
@entelligenceai + *feedback*
Example: @entelligenceai Do not comment on `save_auth` function !
Also you can trigger various commands with the bot by doing
@entelligenceai command
The current supported commands are
config- shows the current configretrigger_review- retriggers the review
More commands to be added soon.
There was a problem hiding this comment.
Pull request overview
This PR refactors the options package to return errors instead of calling os.Exit, making it more testable and idiomatic. The changes include error aggregation via a ConfigError type, comprehensive test coverage for error paths, and a race condition fix in the resettoken package.
Changes:
- Refactored
Parse()to return(*Opts, error)instead of exiting on errors - Introduced
ConfigErrortype to collect and return multiple validation errors - Fixed race condition in resettoken package by adding mutex protection for the global clock variable
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| main.go | Updated to handle error from options.Parse() and exit gracefully with logging |
| internal/options/app.go | Refactored Parse() to return errors; added ConfigError type and deprecated MustParse() for backward compatibility |
| internal/options/app_test.go | Added comprehensive tests for error handling paths including invalid integers, booleans, and missing required fields |
| internal/resettoken/clock.go | Added sync.RWMutex and getClock() helper for thread-safe access to defaultClock |
| internal/resettoken/clock_test.go | Updated test helper to use mutex when setting/resetting test clock |
| internal/resettoken/store.go | Changed IsExpired() to use getClock() instead of directly accessing defaultClock |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 is a significant improvement, refactoring the options parsing to return errors instead of exiting, which greatly enhances testability and follows Go best practices. The introduction of ConfigError to aggregate multiple configuration issues is well-implemented. Additionally, the fix for the race condition in the resettoken package is a crucial improvement for concurrent test execution. I've identified a few minor issues, including a potential bug in integer parsing logic and some dead code, but overall this is excellent work.
- Use strconv.IntSize instead of 16 for proper uint parsing - Suppress FlagSet stderr output with io.Discard - Fix typo: "password can include the password" -> "username" - Fix MustParse comment: says "log.Fatal" but actually prints and exits - Remove unused ErrMissingRequired variable - Update test for decimal value instead of overflow (999999 is valid on 64-bit)
|
Released in v1.2.0 |
Summary
Parse()to return(*Opts, error)instead of callingos.ExitConfigErrortype that collects multiple validation errorsenvIntOrDefaultandenvBoolOrDefaultto accumulate errorsMustParse()for backward compatibilityParse()calls in testsdefaultClockglobal variableMotivation
Libraries should return errors instead of calling
os.Exit, which is not testable and not idiomatic Go. Onlymain()should decide whether to exit.Test plan
go test ./... -racegolangci-lint run🤖 Generated with Claude Code