Skip to content

Conversation

@bytestream
Copy link
Member

Rather than copying to tmp directory within the container, we can simplify the process and just copy directly to the host.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the Docker monolithic backup script to use docker cp for copying files directly from the container to the host, eliminating the intermediate step of copying files to a temporary directory within the container first.

Key Changes:

  • Replaced container-based file operations (docker compose exec ... cp) with docker cp commands that copy directly to the host
  • Changed TEMP_BACKUP_DIR (container path /tmp/tmp-backups/${TIMESTAMP}) to TMP_DIR (host path ${ABS_BACKUP_PATH}/tmp-${TIMESTAMP})
  • Simplified cleanup by only needing to remove the host temporary directory instead of both host and container cleanup

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


# Cleanup.
docker compose exec -u root supportpal bash -c "rm -rf ${TEMP_BACKUP_DIR}/"
# Cleanup (now only needs to clean the host TMP_DIR, container cleanup is minimal)
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

[nitpick] The comment mentions "container cleanup is minimal" but there's no longer any container cleanup happening in this refactored version. The database backup file is explicitly deleted from the container on line 86, which is the only cleanup. Consider updating the comment to be more accurate, e.g., "Cleanup temporary directory from host" or "Cleanup (database backup already removed from container on line 86)".

Suggested change
# Cleanup (now only needs to clean the host TMP_DIR, container cleanup is minimal)
# Cleanup temporary directory from host (database backup already removed from container on line 86)

Copilot uses AI. Check for mistakes.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

Copilot AI and others added 4 commits November 12, 2025 14:41
* Initial plan

* fix: remove trailing slashes from docker cp source paths

Co-authored-by: bytestream <1788397+bytestream@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: bytestream <1788397+bytestream@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: bytestream <1788397+bytestream@users.noreply.github.com>
* Initial plan

* Add error checking to docker cp operations in backup script

Co-authored-by: bytestream <1788397+bytestream@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: bytestream <1788397+bytestream@users.noreply.github.com>
@bytestream bytestream merged commit 57c4810 into 5.x Dec 18, 2025
2 checks passed
@bytestream bytestream deleted the backup branch December 18, 2025 11:05
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.

2 participants