feat: improve docker cleanup helper#797
Conversation
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a new cleanup.sh script and updates cleanup-apt.sh to delegate to it when available, while maintaining standalone functionality for backward compatibility. The review feedback highlights an opportunity to reduce code redundancy between the two scripts and suggests adding documentation to clarify the safety check that conditionally deletes the script directory.
| apt-get purge -qq -y curl wget || true | ||
| apt-get autoremove -qq -y | ||
| apt-get clean -qq | ||
| rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
The apt cleanup logic is duplicated here and in cleanup.sh. According to the repository style guide (Rule 6), code should be simplified to eliminate redundancy. While this duplication allows cleanup-apt.sh to function as a standalone script for backward compatibility (e.g., if only this script is copied into a Docker image), it increases the maintenance burden if the cleanup steps need to change in the future.
References
- Simplify code as much as possible to eliminate redundancy. (link)
| apt-get clean -qq | ||
| rm -rf /var/lib/apt/lists/* | ||
|
|
||
| if [ -z "${WB_KEEP_DOCKER_SCRIPTS:-}" ] && [ "$(basename "${SCRIPT_DIR}")" = "bash" ]; then |
There was a problem hiding this comment.
The check [ "$(basename "${SCRIPT_DIR}")" = "bash" ] acts as a safety guard but relies on a hardcoded directory name. According to the repository style guide (Rule 13), complex or non-obvious logic should be documented. It would be beneficial to add a comment explaining that this check prevents accidental deletion of parent directories if the script is executed from an unexpected location. Additionally, consider if checking for the presence of other helper scripts (e.g., cleanup-apt.sh) would provide a more robust identification of the target directory.
| if [ -z "${WB_KEEP_DOCKER_SCRIPTS:-}" ] && [ "$(basename "${SCRIPT_DIR}")" = "bash" ]; then | |
| # Only remove the directory if it's named "bash" to avoid accidental deletion of system directories | |
| if [ -z "${WB_KEEP_DOCKER_SCRIPTS:-}" ] && [ "$(basename "${SCRIPT_DIR}")" = "bash" ]; then |
References
- Write comments and JSDoc for complex or hard-to-understand code. Explain 'why' in comments. (link)
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
Summary
cleanup.shas the Docker helper cleanup entry pointcleanup.shremove apt artifacts and the copied./bashhelper directory by defaultcleanup-apt.shhelper so downstream Dockerfiles use the broader cleanup nameWhy
cleanup-apt.shname no longer fits when cleanup also owns helper-script removalwb optimizeForDockerBuild --outsidegenerates these helper files together, so downstreams can migrate directly tocleanup.shTesting
bash -n packages/wb/docker/bash/cleanup.shyarn workspace @willbooster/wb start --working-dir /tmp/wb-cleanup-script-check optimizeForDockerBuild --outside/tmp/wb-cleanup-script-check/dist/bashincludescleanup.shyarn verify