- Move network monitoring initialization out of serveDNS() function
- Start network monitoring in a separate goroutine during program startup
- Remove context parameter from monitorNetworkChanges() as it's not used
- Simplify serveDNS() function signature by removing unused context parameter
- Ensure network monitoring starts only once during initial run, not on reload
This change improves separation of concerns by isolating network monitoring
from DNS serving logic, and prevents potential issues with multiple
monitoring goroutines if starting multiple listeners.
Add comprehensive test suite for all Cobra CLI commands in cmd/cli/commands_test.go.
The test suite includes:
- Basic command structure validation
- Service command creation and subcommand testing
- Help and version command functionality
- Error handling for invalid flags
- Flag validation (verbose, silent)
- Command execution and argument handling
- Subcommand validation
Key features:
- Uses sync.Once for thread-safe CLI initialization
- Tests the actual global rootCmd instead of isolated instances
- Provides realistic test coverage of the application's command structure
- All tests pass and project builds successfully
- Restore HTTP 400 status handling in log viewing that was lost during refactoring
- Restore service installation check in restart command that was missing after refactoring
- Ensures uninstall command has same flag functionality as stop command
- Fixes inconsistency where uninstallCmdAlias had flags but main uninstallCmd did not
Move uninstallCmd.AddCommand() to match the order of ValidArgs array
definition, ensuring the command addition order aligns with the
valid arguments list order.
- Update all Init*Cmd function signatures to accept rootCmd parameter:
* InitServiceCmd(rootCmd *cobra.Command)
* InitClientsCmd(rootCmd *cobra.Command)
* InitLogCmd(rootCmd *cobra.Command)
* InitUpgradeCmd(rootCmd *cobra.Command)
* InitRunCmd(rootCmd *cobra.Command)
* InitInterfacesCmd(rootCmd *cobra.Command)
- Update function calls in cli.go to pass rootCmd parameter
- Update InitInterfacesCmd call in commands_service.go
Benefits:
- Eliminates global state dependency on rootCmd variable
- Makes dependencies explicit in function signatures
- Improves testability by allowing different root commands
- Better encapsulation and modularity
- Replace all direct newService() calls with ServiceCommand initialization
- Update command constructors to use ServiceCommand instead of ServiceManager
- Simplify LogCommand and UpgradeCommand structs by removing serviceManager field
- Remove unused global svcConfig variable from prog.go
- Improve consistency and centralize service creation logic
This change establishes a consistent pattern for service operations across
the codebase, making it easier to maintain and extend service-related
functionality.
- Split initializeServiceManager into two methods:
* initializeServiceManager(): Simple method using default configuration
* initializeServiceManagerWithServiceConfig(): Advanced method for custom config
- Simplify NewServiceCommand() to return *ServiceCommand without error
- Update all service command methods to use appropriate initialization:
* Start: Uses initializeServiceManagerWithServiceConfig() for custom args
* Stop/Restart/Reload/Status/Uninstall: Use simple initializeServiceManager()
- Remove direct access to sc.serviceManager.svc/prog in favor of lazy initialization
- Improve separation of concerns and reduce code duplication
Remove rootCmd.AddCommand call from createStartCommands function.
The function should only create and return commands, not add them
to the root command hierarchy. This responsibility belongs to the
caller (InitServiceCmd).
This change improves:
- Separation of concerns: function has single responsibility
- Testability: no hidden side effects
- Flexibility: caller controls command registration
- Clean architecture: follows principle of no hidden dependencies
- Move ServiceCommand.Start to commands_service_start.go
- Move ServiceCommand.Stop to commands_service_stop.go
- Move ServiceCommand.Restart to commands_service_restart.go
- Move ServiceCommand.Reload to commands_service_reload.go
- Move ServiceCommand.Status to commands_service_status.go
- Move ServiceCommand.Uninstall to commands_service_uninstall.go
- Move createStartCommands to commands_service_start.go
- Clean up imports in commands_service.go
- Remove all method implementations from main service file
This refactoring improves code organization by:
- Separating concerns into focused files
- Making navigation easier for developers
- Reducing merge conflicts between different commands
- Following consistent modular patterns
- Reducing commands_service.go from ~650 lines to ~50 lines
Each method is now co-located with its related functionality,
making the codebase more maintainable and easier to understand.
Add missing selfDeleteExe() call and supportedSelfDelete check that were
present in the original initUninstallCmd function. This ensures the
uninstall command properly handles self-deletion of the binary when
cleanup is enabled.
The original logic included:
- selfDeleteExe() call for self-deletion
- supportedSelfDelete check for platform-specific behavior
- Proper error handling and logging
This completes the porting of all functionality from the original
initUninstallCmd to the new ServiceCommand.Uninstall method.
Rename service_manager.go to commands_service_manager.go to follow the
established naming pattern with other command files.
Remove the unused CommandRunner interface from commands.go since it's not
being used anywhere in the codebase. Clean up unused imports.
This improves consistency in file naming and removes dead code.
Create commands_run.go following the same modular pattern as other
command files. Move initRunCmd logic to InitRunCmd function with
consistent naming and complete functionality preservation.
Update cli.go to use InitRunCmd() instead of initRunCmd() and clean
up commands.go by removing the old function and unused imports.
This completes the modular refactoring pattern where each command type
has its own dedicated file with focused responsibility.
Remove all unused service command functions (initStartCmd, initStopCmd,
initRestartCmd, initReloadCmd, initStatusCmd, initUninstallCmd,
initInterfacesCmd, initClientsCmd, initUpgradeCmd, initServicesCmd)
from commands.go since they have been replaced by modular implementations
in dedicated files. Keep only essential functions: CommandRunner interface,
ServiceManager struct, NewServiceManager function, Status method,
initRunCmd function, and filterEmptyStrings function.
Update cli.go to use InitClientsCmd() and InitUpgradeCmd() instead of
the old init functions. Clean up unused imports and simplify
filterEmptyStrings implementation.
This reduces commands.go from 1202 lines to 103 lines (91% reduction)
and eliminates code duplication while improving maintainability.
Replace individual service command initialization with unified InitServiceCmd()
that creates a complete service command hierarchy. Port all original logic
from initStartCmd, initStopCmd, initRestartCmd, initReloadCmd, initStatusCmd,
and initUninstallCmd into ServiceCommand methods with proper dependency injection.
Key changes:
- Port complete Start logic including config validation, service installation,
DNS management, and self-check functionality
- Port complete Stop logic with deactivation pin validation and DNS cleanup
- Port complete Restart logic with config validation and DNS restoration
- Port complete Reload logic with HTTP status handling and restart fallback
- Port complete Status logic with proper exit codes
- Port complete Uninstall logic with cleanup file removal
- Add all necessary flags to service commands (iface, pin, etc.)
- Use InitInterfacesCmd() for interfaces subcommand
- Simplify cli.go by replacing multiple init calls with single InitServiceCmd()
This refactoring eliminates code duplication, improves maintainability, and
ensures all service commands have their complete original functionality.
Remove the old initLogCmd function from commands.go and update cli.go
to use the new InitLogCmd function from commands_log.go. Complete
the log command refactoring by adding the missing InitLogCmd function
with proper command structure and error handling.
Port all special logic from original alias commands:
- startCmdAlias: custom Args validation, startOnly logic, iface handling
- stopCmdAlias: iface flag handling and argument passing
- restartCmdAlias: simple delegation to restartCmd.RunE
- reloadCmdAlias: simple delegation to reloadCmd.RunE
- statusCmdAlias: simple delegation to statusCmd.RunE
- uninstallCmdAlias: iface flag handling and argument passing
All aliases now have exact same behavior as original implementation
including proper flag inheritance and argument handling.
Create separate file for interfaces command handling to improve code organization.
Add InterfacesCommand struct with ListInterfaces method that handles the
logic to list current system interfaces.
Create separate file for clients command handling to improve code organization.
Add ClientsCommand struct with ListClients method that includes all original logic:
service status checks, HTTP requests, source mapping, metrics handling, and table
formatting. Includes InitClientsCmd function that creates proper command hierarchy
with clients parent command and list sub-command.
Create separate file for upgrade command handling to improve code organization.
Add UpgradeCommand struct with Upgrade method that includes all original logic:
channel management, service restart, rollback handling, and version verification.
Includes InitUpgradeCmd function with proper argument validation and privilege checks.
Create separate file for service command handling to improve code organization.
Add ServiceCommand struct with Install, Uninstall, Start, Stop, and Status
methods to handle service operations with proper error handling and dependency
injection.
Create separate file for log command handling to improve code organization.
Add LogCommand struct with SendLogs and ViewLogs methods to handle
log-related operations with proper error handling and dependency injection.
- Add NoticeLevel constant using zapcore.WarnLevel value (1)
- Implement custom level encoders (noticeLevelEncoder, noticeColorLevelEncoder)
- Update Notice() method to use custom level
- Add "notice" case to log level parsing in main.go
- Update encoder configurations to handle NOTICE level properly
- Add comprehensive test (TestNoticeLevel) to verify behavior
The NOTICE level provides visual distinction from INFO and ERROR levels,
with cyan color in development and proper level filtering. When log level
is set to NOTICE, it shows NOTICE and above (WARN, ERROR) while filtering
out DEBUG and INFO messages.
Note: NOTICE and WARN share the same numeric value (1) due to zap's
integer-based level system, so both display as "NOTICE" in logs for
visual consistency.
Usage:
- logger.Notice().Msg("message")
- log_level = "notice" in config
- Supports structured logging with fields
- Add condition to skip port 53 attempts when using zero IP address
- Improve error logging by using structured error field instead of string formatting
- Remove redundant error information from log message format
The changes prevent unnecessary port 53 binding attempts when using zero IP
addresses and improve log readability by using zap's structured error fields.
Replace github.com/rs/zerolog with go.uber.org/zap throughout the codebase
to improve performance and provide better structured logging capabilities.
Key changes:
- Replace zerolog imports with zap and zapcore
- Implement custom Logger wrapper in log.go to maintain zerolog-like API
- Add LogEvent struct with chained methods (Str, Int, Err, Bool, etc.)
- Update all logging calls to use the new zap-based wrapper
- Replace JSON encoders with Console encoders for better readability
Benefits:
- Better performance with zap's optimized logging
- Consistent structured logging across all components
- Maintained zerolog-like API for easy migration
- Proper field context preservation for debugging
- Multi-core logging architecture for better output control
All tests pass and build succeeds.
- Add explicit foundDefaultRoute boolean variable to track default route discovery
- Initialize foundDefaultRoute to false and set to true only in success case
- Replace tautological condition `err == nil` with meaningful `foundDefaultRoute` check
- Fixes "tautological condition: nil == nil" linter error
The error occurred because err was being reused from net.Interfaces() call,
making the condition always true. Now we explicitly track whether a default
route was successfully found.
- Split handleRecovery into focused helper methods for better maintainability:
* shouldStartRecovery: handles recovery cancellation logic
* createRecoveryContext: manages recovery context and cleanup
* prepareForRecovery: removes DNS settings and initializes OS resolver
* completeRecovery: resets upstream state and reapplies DNS settings
* reinitializeOSResolver: reinitializes OS resolver with proper logging
* Update handleRecovery documentation to reflect new orchestration role
- Improve tests:
* Add newTestProg helper to reduce test setup duplication
* Write comprehensive table-driven tests for all recovery methods
This refactoring improves code maintainability, testability, and reduces
complexity while maintaining the same recovery behavior. Each method now
has a single responsibility and can be tested independently.
- Add filterEmptyStrings utility function for consistent string filtering
- Replace inline slices.DeleteFunc calls with filterEmptyStrings
- Apply filtering to osArgs in addition to command args
- Improves code readability and reduces duplication
- Uses slices.DeleteFunc internally for efficient filtering
- Move version checking logic to shouldUpgrade for testability
- Move upgrade command execution to performUpgrade
- selfUpgradeCheck now composes these two for clarity
- Update and expand tests: focus on logic, not side effects
- Improves maintainability, testability, and separation of concerns
Logging there should use Log function to include the request ID if
present. Changes were made unintentionally during the refactoring to
eliminate usage of global logger.
This commits message restores the correct/old behavior.
Split the long proxy method into several smaller methods to improve maintainability
and testability. Each new method has a single responsibility:
- initializeUpstreams: handles upstream configuration setup
- tryCache: manages cache lookup logic
- tryUpstreams: coordinates upstream query attempts
- processUpstream: handles individual upstream query processing
- handleAllUpstreamsFailure: manages failure scenarios
- checkCache: performs cache checks and retrieval
- serveStaleResponse: handles stale cache responses
- shouldContinueWithNextUpstream: determines if failover is needed
- prepareSuccessResponse: formats successful responses
This refactoring:
- Reduces cognitive complexity
- Improves code testability
- Makes the DNS proxy logic flow clearer
- Isolates error handling and edge cases
- Maintains existing functionality
No behavioral changes were made.
This change improves compatibility with newer UniFi OS versions while
maintaining backward compatibility with UniFi OS 4.2 and earlier.
The refactoring also reduces code duplication and improves maintainability
by centralizing dnsmasq configuration path logic.
Break down the large DNS handling function into smaller, focused functions
with clear responsibilities:
- Extract handleDNSQuery from serveDNS handler function
- Create dedicated startListeners function for listener management
- Add standardQueryRequest struct to encapsulate query parameters
- Split special domain handling into separate function
- Add descriptive comments for each new function
- Improve variable names for better clarity (e.g., startTime vs t)
This refactoring improves code maintainability and readability without
changing the core DNS proxy functionality.
Move getDNS type definition from dns.go to os_linux.go where it is used.
Remove the now-empty dns.go file. This change improves code organization
by keeping platform-specific types with their implementations.
Add context parameter to validInterfacesMap for better error handling and
logging. Move Windows-specific network adapter validation logic to the
ctrld package. Key changes include:
- Add context parameter to validInterfacesMap across all platforms
- Move Windows validInterfaces to ctrld.ValidInterfaces
- Improve error handling for virtual interface detection on Linux
- Update all callers to pass appropriate context
This change improves error reporting and makes the interface validation
code more maintainable across different platforms.
Improve documentation for Test_prog_parseResolvConfNameservers to clarify that
the old implementation was removed as part of code deduplication effort. The code
for handling resolv.conf was unified into the resolvconffile package to provide
a consistent interface across the codebase.
This change provides better context for future developers about why the
refactoring was done and what benefits it brings.
Move client information related functions from client_info_*.go to desktop_*.go files
to better organize platform-specific code and separate desktop functionality from
shared code.
No functional changes.
Make nameserver resolution functions more consistent and accessible:
- Rename currentNameserversFromResolvconf to CurrentNameserversFromResolvconf
- Move function to public API for better reusability
- Update all internal references to use the new public API
- Add comprehensive godoc comments for nameserver functions
- Improve code organization by centralizing DNS resolution logic
This change makes the nameserver resolution functionality more maintainable
and easier to use across different parts of the codebase.
By adding a logger field to "prog" struct, and use this field inside its
method instead of always accessing global mainLog variable. This at
least ensure more consistent usage of the logger during ctrld prog
runtime, and also help refactoring the code more easily in the future
(like replacing the logger library).
So setting up logging for ctrld binary and ctrld packages could be done
more easily, decouple the required setup for interactive vs daemon
running.
This is the first step toward replacing rs/zerolog libary with a
different logging library.
