Enhance Docker permission detection and improve error messages (Fixes #34)#334
Enhance Docker permission detection and improve error messages (Fixes #34)#334roman-y-wu wants to merge 9 commits intoosrf:mainfrom
Conversation
- Enhanced ShmSize extension with build-time shm-size support via --shm-size-build - Added CpuLimits extension providing --cpus argument for runtime CPU limits - Added MemoryLimits extension providing --memory/-m argument for runtime memory limits - Extended core infrastructure to support docker build arguments from extensions - Added comprehensive input validation for memory and CPU formats - Updated documentation and added test coverage for all new functionality - Improved .gitignore with comprehensive Python and development file patterns These extensions expose docker build and runtime resource limit arguments (shm-size, cpus, memory) to rocker users, providing better control over container resource allocation.
- Move ShmSize, CpuLimits, and MemoryLimits to separate constraint_extensions.py file - Create reusable validate_memory_format() helper function to avoid code duplication - Enhance memory validation to support Docker byte-value formats (2b, 1024kb, 300m, 1gb) - Update CPU error message to "Expected a floating point number (e.g., '1.5')" - Update setup.py entry points to reference new constraint_extensions module Addresses all feedback from tfoote in PR osrf#331 review comments.
… permission issues
tfoote
left a comment
tfoote
left a comment
There was a problem hiding this comment.
This approach seems to be relatively fragile and doing a lot of string parsing. It seems to me that it might make sense to simply do a check which will validate directly the measuable things that might block this. For example testing directly if docker is on the path. or check if the user is in the docker group. Those things can be done to measure the actual error state and aren't fragile to error strings which might evolve across.
I also haven't looked deeper but are any of these caught in subclasses of DockerException that could be detected without string parsing?
| print("ERROR: %s" % ex) | ||
| if ex.suggested_fix: | ||
| print("\n%s" % ex.suggested_fix) | ||
| return 1 |
There was a problem hiding this comment.
Please use parser.error here instead of printing and returning.
And please add a descriptive context for the Error not just ERROR:
| """ | ||
| error_str = str(exception).lower() | ||
|
|
||
| # Check for common permission-related error patterns |
There was a problem hiding this comment.
How did you create this list?
| return True, error_message, suggested_fix | ||
|
|
||
| # Check for Docker daemon not running | ||
| daemon_indicators = [ |
There was a problem hiding this comment.
How did you create this one too?
|
Superseded by #343 |
2 participants
issue: #34
Solution
Testing