Add Docker environment check #52
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughCentralizes Docker environment validation and shifts platform-string discovery to rely on an already-running container; introduces undo-handler based container cleanup, adds a container sysroot mount constant, and changes sysroot APIs to return errors and use the container mount path. Changes
Sequence Diagram(s)sequenceDiagram
participant Operator as Operator
participant Mode as Mode (App/Package/Sysroot)
participant Docker as DockerRun
participant Sysroot as SysrootService
participant Platform as PlatformString
Operator->>Mode: start build
Mode->>Docker: checkDockerEnvironmentAndDeterminePlatformString(image, port)
Docker->>Sysroot: CreateBaseSysrootDir() / prepare test sysroot
Sysroot-->>Docker: base sysroot path
Docker->>Docker: run container, mount sysroot -> rgba(255,165,0,0.5)
Docker->>Docker: create SSH credentials
Docker->>Docker: checkDockerEnvironment (permissions, container paths)
Docker->>Platform: determinePlatformString(on running container)
Platform-->>Docker: PlatformString
Docker-->>Mode: return PlatformString + undo handler
Mode->>Mode: proceed with build using PlatformString
Mode->>Docker: defer undo handler (stop & remove container)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Contributor
Author
|
Works as expected on Ubuntu 24.04 (without SELinux) and Fedora 43 (with SELinux). All tests have passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sysroot/default_test.go (1)
111-123:⚠️ Potential issue | 🟡 MinorAssert CreateSysrootDir() errors in the test.
CreateSysrootDir now returns an error; the test should fail explicitly if it can’t create the directory.
✅ Suggested test fix
- defaultSysroot.CreateSysrootDir() + if err := defaultSysroot.CreateSysrootDir(); err != nil { + t.Fatalf("CreateSysrootDir failed - %s", err) + }
🤖 Fix all issues with AI agents
In `@cmd/bap-builder/PackageMode.go`:
- Around line 413-415: The shell command string in ssh.ShellEvaluator (variable
shellEvaluator, field Commands) is missing spaces before the path for the `test
-r` and `test -w` checks; update the command that builds from testDir to insert
a space after `test -r` and `test -w` (e.g., `"mkdir " + testDir + " && test -r
" + testDir + " && test -w " + testDir + " && rmdir " + testDir"`) so the tests
run with valid shell syntax.
- Around line 405-407: The docstring for checkDockerEnvironment in
PackageMode.go is incorrect (it describes downloading/cloning); update the
comment above the checkDockerEnvironment function to accurately state that it
validates Docker environment permissions and availability (e.g., checks Docker
daemon connectivity, user permissions, and required capabilities) and remove any
copy-pasted references to downloading packages or updating submodules so the
comment matches the function's actual behavior and checks.
- Around line 440-444: Remove the redundant assignment to
defaultDocker.ImageName after creating the Docker instance: the call to
prerequisites.CreateAndInitialize[docker.Docker](dockerImageName, dockerPort)
already sets ImageName via docker.Docker.FillDynamic, so delete the line that
assigns defaultDocker.ImageName = dockerImageName; keep the CreateAndInitialize
call and subsequent logic unchanged.
In `@internal/build/Build.go`:
- Around line 159-166: CreateSysrootDir()'s error is being ignored; update the
block that checks build.sysroot to capture and return any error from
build.sysroot.CreateSysrootDir() before calling GetSysrootPath and SetVolume. In
the code around Build.sysroot, call CreateSysrootDir(), check its error (e.g.,
if err := build.sysroot.CreateSysrootDir(); err != nil), and return that error
(or wrap it with context) so subsequent calls to build.sysroot.GetSysrootPath()
and build.Docker.SetVolume() don't run on a failed setup.
In `@internal/sysroot/Sysroot.go`:
- Around line 18-20: The constant sysrootDirPermissions is set to world‑writable
0777 which is insecure; change sysrootDirPermissions to a safer default such as
0755 (or 0775 if group write is needed) and add a comment noting the choice, and
optionally make permissions configurable (e.g., via a setter or config value
used by the code that creates the directory) so callers can override when
broader access is required; update any code that relies on sysrootDirPermissions
to read the configurable value instead of the hardcoded 0777.
- Around line 175-188: The comment for CreateSysrootDir is outdated (it says it
panics) even though the function now returns an error; update the function
comment to accurately state that CreateSysrootDir creates the sysroot directory
and returns an error on failure instead of panicking, mentioning the Sysroot
receiver and that it uses GetSysrootPath and os.MkdirAll to create the directory
and propagate errors.
- Around line 138-158: The comment for CreateBaseSysrootDir is outdated (it
claims a panic occurs) — update the function doc to accurately state that
CreateBaseSysrootDir returns an error on failure instead of panicking; mention
that it uses GetBaseSysrootPath and os.MkdirAll and that callers should handle
the returned error.
🧹 Nitpick comments (2)
internal/docker/DockerRun.go (1)
42-58: Avoid fixed sleep and surface cleanup failures.Run() is synchronous, so the fixed 200ms delay is likely unnecessary. Also, returning the first stop/remove error would let non-deferred callers react to cleanup failures.
♻️ Suggested tweak
func (args *DockerRun) GetUndoHandler() func() error { return func() error { - // Waiting for docker run command to get container id - time.Sleep(200 * time.Millisecond) + if args.containerId == "" { + return nil + } dockerStop := (*DockerStop)(args) dockerRm := (*DockerRm)(args) logger := log.GetLogger() - err := dockerStop.Stop() - if err != nil { + if err := dockerStop.Stop(); err != nil { logger.Error("Can't stop container - %s", err) + return err } - err = dockerRm.RemoveContainer() - if err != nil { + if err := dockerRm.RemoveContainer(); err != nil { logger.Error("Can't remove container - %s", err) + return err } return nil } }internal/sysroot/Sysroot.go (1)
194-199: Consider logging when GetSysrootPath fails.Returning “empty” on path resolution errors can hide real issues; a warning log would make this easier to debug.
🔎 Optional logging
sysrootPath, err := sysroot.GetSysrootPath() if err != nil { + log.GetLogger().Warn("Cannot resolve sysroot path: %s", err) return true }
mikusaq
force-pushed
the
BAF-1299/check-docker-permissions
branch
from
7565df8 to
d6d2e59
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/bap-builder/PackageMode.go`:
- Around line 456-492: The deferred container cleanup handler is obtained before
dockerRun.Run and will run even if Run fails, causing spurious "container not
found" errors; move the call to dockerRun.GetUndoHandler() and the defer
removeHandler() to immediately after dockerRun.Run() returns successfully (i.e.,
only call GetUndoHandler() and defer it when dockerRun.Run() returns nil) inside
checkDockerEnvironmentAndDeterminePlatformString so cleanup is registered only
for actually created containers (keep references to dockerRun, GetUndoHandler,
removeHandler and ensure error handling still returns on Run failure).
In `@internal/docker/DockerRun.go`:
- Around line 42-58: GetUndoHandler currently always sleeps and runs Stop/Remove
even if the DockerRun's containerId is empty; add an early guard in
DockerRun.GetUndoHandler that checks the receiver's containerId (e.g.,
args.containerId or the appropriate field on DockerRun) and if it's empty return
a no-op func immediately to avoid the 200ms sleep and calling
DockerStop.Stop/DockerRm.RemoveContainer with an empty id; keep existing error
logging for non-empty cases and do not call Stop/Remove when containerId is
blank.
🧹 Nitpick comments (2)
cmd/bap-builder/PackageMode.go (1)
405-429: Include underlying SSH error in diagnostics.Both failure branches discard
err, which makes triage harder. Consider logging and wrapping the underlying error.🧩 Suggested tweak
err := shellEvaluator.RunOverSSH(credentials) if err != nil { - logger.ErrorIndent("Cannot create directories, or read or write to them inside Docker container") - return fmt.Errorf("invalid Docker environment") + logger.ErrorIndent("Cannot create directories, or read or write to them inside Docker container - %s", err) + return fmt.Errorf("invalid Docker environment: %w", err) } @@ err = shellEvaluator.RunOverSSH(credentials) if err != nil { - logger.ErrorIndent("Cannot read volume directory inside Docker container") - return fmt.Errorf("invalid Docker environment") + logger.ErrorIndent("Cannot read volume directory inside Docker container - %s", err) + return fmt.Errorf("invalid Docker environment: %w", err) }internal/sysroot/Sysroot.go (1)
194-200: LogGetSysrootPathfailures for visibility.Right now failures are silent and return
true; a warning helps diagnose path/permission issues.🧩 Suggested tweak
sysrootPath, err := sysroot.GetSysrootPath() if err != nil { + log.GetLogger().Warn("Cannot resolve sysroot path: %s", err) return true }
mikusaq
force-pushed
the
BAF-1299/check-docker-permissions
branch
from
d6d2e59 to
bc131ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/build/Build.go (1)
205-214:⚠️ Potential issue | 🟡 MinorDefer cleanup only after a successful container start.
IfdockerRun.Run()fails, the deferred cleanup may attempt to stop/remove a non‑existent container and emit noisy errors.🧹 Proposed fix
dockerRun := (*docker.DockerRun)(build.Docker) - removeHandler := dockerRun.GetUndoHandler() - defer removeHandler() - logger.InfoIndent("Starting docker container") err = dockerRun.Run() if err != nil { return err, false } + removeHandler := dockerRun.GetUndoHandler() + defer removeHandler()
🤖 Fix all issues with AI agents
In `@cmd/bap-builder/PackageMode.go`:
- Around line 436-453: The function prepareDockerEnvironmentCheck calls
defaultDocker.SetVolume(sysrootPath, constants.ContainerSysrootPath) but ignores
any error; validate and propagate SetVolume's error so a failed volume mount
fails the environment check. Update prepareDockerEnvironmentCheck to capture the
return from defaultDocker.SetVolume, return that error (or wrap it with context)
before returning defaultDocker, ensuring callers receive the failure from
SetVolume.
🧹 Nitpick comments (1)
internal/sysroot/Sysroot.go (1)
194-198: Log GetSysrootPath failures before treating sysroot as empty.
Silent fallback makes debugging permission/FS issues harder.🔍 Proposed tweak
sysrootPath, err := sysroot.GetSysrootPath() if err != nil { + log.GetLogger().Warn("Cannot resolve sysroot path: %s", err) return true }
mikusaq
force-pushed
the
BAF-1299/check-docker-permissions
branch
from
bc131ce to
8243ac3
Compare
Remove unnecessary sleep and add check for empty containerID.
mikusaq
force-pushed
the
BAF-1299/check-docker-permissions
branch
from
8243ac3 to
e22de16
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
os.MkDirAllusageSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.