Skip to content

install: Only remove BlueOS containers and images - #4355

Open
patrickelectric wants to merge 1 commit into
bluerobotics:masterfrom
patrickelectric:install-remove-blueos-only
Open

install: Only remove BlueOS containers and images#4355
patrickelectric wants to merge 1 commit into
bluerobotics:masterfrom
patrickelectric:install-remove-blueos-only

Conversation

@patrickelectric

Copy link
Copy Markdown
Member

NO_CLEAN wipe was stopping every container and pruning all unused images.
Fix #2747

@github-actions

Copy link
Copy Markdown

Automated PR Review

0. Summary

  • Verdict: MINOR SUGGESTIONS ✏️

Scopes the NO_CLEAN-gated cleanup block in install/install.sh to BlueOS-only containers/images instead of stopping every container on the host and pruning every unused image. Nice, tightly-scoped fix for #2747 that prevents the installer from trampling unrelated Docker workloads.

1. Correctness & Implementation Bugs

  • 1.1 [minor] install/install.sh:205 — the outer guard [[ $(docker ps -a -q) ]] still enumerates all containers, not just BlueOS ones. If the host has non-BlueOS containers but no BlueOS containers (partial/failed prior install, or a fresh box that happens to have unrelated containers), the block enters, docker stop is invoked with an empty argument list (from the new filter), Docker exits non-zero, and because set -e is active from line 54 the subshell aborts before reaching the docker rmi line. Any leftover *blueos-core* / *blueos-bootstrap* images then never get cleaned. Consider making the guard use the same filter, e.g.
    blueos_containers=$(docker ps -a -q --filter name=blueos)
    blueos_images=$(docker images -q --filter reference='*blueos-core*' --filter reference='*blueos-bootstrap*')
    [[ -n $blueos_containers || -n $blueos_images ]] && (
        [[ -n $blueos_containers ]] && { docker stop $blueos_containers; docker rm $blueos_containers; }
        [[ -n $blueos_images ]] && docker rmi $blueos_images
    ) || true
    That also avoids the noisy "docker stop requires at least 1 argument" stderr when there is nothing to remove.
  • 1.2 [minor] install/install.sh:210docker rmi $(docker images -q --filter reference='*blueos-core*') $(docker images -q --filter reference='*blueos-bootstrap*') errors out with "'docker rmi' requires at least 1 argument" when both subcommands return empty (common on a first-time install where only the raw core image was pulled but not tagged with blueos-*, or when the user pre-built one but not the other). || true masks the exit code, but the stderr noise misleads users. docker images accepts multiple --filter reference= flags — combining them into one invocation both fixes this and reads cleaner:
    images=$(docker images -q --filter reference='*blueos-core*' --filter reference='*blueos-bootstrap*')
    [[ -n $images ]] && docker rmi $images

3. Security

  • 3.5 [nit] Change touches install/install.sh, which is on the "reviewer must eyeball this" list — but the direction here is less destructive, not more. No new network calls, no new privileged operations, no new environment variables. Filters use fixed literal patterns (blueos, *blueos-core*, *blueos-bootstrap*) with no user input, so there is no shell-injection surface.

6. Code Quality & Style

  • 6.1 [nit] install/install.sh:210 — the two positional args to docker rmi are functionally "the BlueOS images list". Extracting into a single variable (see 1.2) both fixes the empty-args edge case and makes the intent read at a glance without needing a comment.

Generated by PR Review Bot. This is advisory, a human reviewer must still approve.

@patrickelectric
patrickelectric requested a review from a team August 29, 2026 12:53

@joaomariolago joaomariolago left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend to take a look at the AI review points [1.1 and 1.2], specifically the entry guard to the remove code that currently checks for any container.

Comment thread install/install.sh Outdated
docker rm $(docker ps -a -q)
docker image prune -af
docker rm $(docker ps -a -q --filter name=blueos)
docker rmi $(docker images -q --filter reference='*blueos-core*') $(docker images -q --filter reference='*blueos-bootstrap*')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we need to add a forced remove here to keep compatibility with the old code.

@patrickelectric

Copy link
Copy Markdown
Member Author

@joaomariolago check now

@patrickelectric
patrickelectric force-pushed the install-remove-blueos-only branch 2 times, most recently from 3ddc8c0 to 83d430f Compare September 3, 2026 15:40
@patrickelectric
patrickelectric force-pushed the install-remove-blueos-only branch 2 times, most recently from 755c241 to 6ed51b1 Compare September 3, 2026 15:50
NO_CLEAN wipe was stopping every container and pruning all unused
images.

Fix bluerobotics#2747

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
@patrickelectric
patrickelectric force-pushed the install-remove-blueos-only branch from 6ed51b1 to c812607 Compare September 3, 2026 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

installer: Only remove blueos-related images

2 participants