Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enables 'rsync' as a copy tool #88

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Enables 'rsync' as a copy tool #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a97226
Allow restoring directories
nelsonjr Dec 28, 2024
c16e461
Allow selecting 'rsync' as copy tool
nelsonjr Dec 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion backends/cache_fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,25 @@ DIR="$(cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd)"
restore_cache() {
local from="${CACHE_FOLDER}/$1"
local to="$2"
local fs_copy_tool="$3"

wait_and_lock "${from}"
# shellcheck disable=2064 # actually want variable interpolated here
trap "release_lock_folder '${from}'" SIGINT SIGTERM SIGQUIT

cp -a "$from" "$to"
if [ -d "$from" ]; then
if [ "$fs_copy_tool" = "rsync" ]; then
rsync -a --delete "${from}/." "$to" # copy as folder
else
cp -a "${from}/." "$to" # copy as folder
fi
else
if [ "$fs_copy_tool" = "rsync" ]; then
rsync -a --delete "$from" "$to"
else
cp -a "$from" "$to"
fi
fi
Comment on lines +26 to +38
Copy link
Contributor

Choose a reason for hiding this comment

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

a cleaner way to do this would be:

Suggested change
if [ -d "$from" ]; then
if [ "$fs_copy_tool" = "rsync" ]; then
rsync -a --delete "${from}/." "$to" # copy as folder
else
cp -a "${from}/." "$to" # copy as folder
fi
else
if [ "$fs_copy_tool" = "rsync" ]; then
rsync -a --delete "$from" "$to"
else
cp -a "$from" "$to"
fi
fi
if [ "$fs_copy_tool" = "rsync" ]; then
copy_exec=(rsync -a --delete)
else
copy_exec=(cp -a)
fi
"${copy_exec[@]}" "${from}" "${to}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points. I will update the code and send another one for review. I did not fork the backend because common logic must be duplicated (and kept in sync).

I like the array trick ;-)

However, regarding this:

it breaks single-file restoring (because you changed ${from} to ${from}/.

If you note, that's under the "if -d," which tests if it is a directory. Only if it's a directory, we add a "/." to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is only implemented for the restore and not the save

On purpose. For save, we always allocate a new (empty) folder, and rsync does not gain over cp for empty folders. We can use the same tool for symmetry, but performance will be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is missing documentation
it is missing tests

Good points. I will add those!


release_lock "${from}"
}
Expand Down
10 changes: 8 additions & 2 deletions hooks/post-checkout
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ if ! validate_compression "${COMPRESS}"; then
exit 1
fi

FS_COPY_TOOL=$(plugin_read_config FS_COPY_TOOL 'cp')
if ! validate_fs_copy_tool "${FS_COPY_TOOL}"; then
echo "+++ 🚨 Invalid value for fs_copy_tool option"
exit 1
fi

build_key "${MAX_LEVEL}" "${RESTORE_PATH}" >/dev/null # to validate the level

SORTED_LEVELS=(file step branch pipeline all)
Expand All @@ -45,10 +51,10 @@ for CURRENT_LEVEL in "${SORTED_LEVELS[@]}"; do

if compression_active; then
TEMP_PATH=$(mktemp)
backend_exec get "${KEY}" "${TEMP_PATH}"
backend_exec get "${KEY}" "${TEMP_PATH}" "cp"
uncompress "${TEMP_PATH}" "${RESTORE_PATH}"
else
backend_exec get "${KEY}" "${RESTORE_PATH}"
backend_exec get "${KEY}" "${RESTORE_PATH}" "${FS_COPY_TOOL}"
fi

exit 0
Expand Down
13 changes: 13 additions & 0 deletions lib/plugin.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,17 @@ function plugin_read_config() {
local var="BUILDKITE_PLUGIN_${PLUGIN_PREFIX}_${1}"
local default="${2:-}"
echo "${!var:-$default}"
}

function validate_fs_copy_tool() {
local FS_COPY_TOOL="$1"

VALID_FS_COPY_TOOL=(cp rsync)
for VALID in "${VALID_FS_COPY_TOOL[@]}"; do
if [ "${FS_COPY_TOOL}" = "${VALID}" ]; then
return 0
fi
done

return 1
}
5 changes: 5 additions & 0 deletions plugin.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ configuration:
- type: array
items:
$ref: '#/$defs/valid_levels'
fs_copy_tool:
type: string
enum:
- cp
- rsync
additionalProperties: false
anyOf:
- required: [ path, save ]
Expand Down