-
Notifications
You must be signed in to change notification settings - Fork 3
[ACIX-1225] Create dda env dev fs import and export commands
#233
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.
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
base: main
Are you sure you want to change the base?
Conversation
bffa189 to
b055041
Compare
b055041 to
dbe0c30
Compare
dbe0c30 to
95901d4
Compare
…ort_from_dir` in dev envs
dda env dev fs import and export commands
eb939cb to
721dada
Compare
There was a problem hiding this 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 implements dda env dev fs import and dda env dev fs export commands that enable easy file transfer between the host filesystem and development environments. The feature is designed to be extensible to different environment types (not just Linux containers) by implementing the functionality through the dev env interface.
Key Changes:
- Added abstract
export_filesandimport_filesmethods to the dev environment interface - Implemented core file operation logic in
src/dda/env/dev/fs.pythat's agnostic to environment type - Created CLI commands for export, import, and an internal localimport command for use inside containers
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/dda/env/dev/interface.py |
Added abstract methods export_files and import_files to the dev environment interface |
src/dda/env/dev/fs.py |
New module containing environment-agnostic file operation logic (import_from_dir, determine_final_copy_target, handle_overwrite) |
src/dda/env/dev/types/linux_container.py |
Implemented export_files and import_files methods for Linux containers using docker cp and shared directories; added per-instance shared directory mount |
src/dda/utils/fs.py |
Enhanced temp_directory function to accept optional dir parameter for controlling where temporary directories are created |
src/dda/cli/env/dev/fs/__init__.py |
New CLI group for filesystem operations |
src/dda/cli/env/dev/fs/export/__init__.py |
CLI command for exporting files from dev environments to host |
src/dda/cli/env/dev/fs/import/__init__.py |
CLI command for importing files from host to dev environments |
src/dda/cli/env/dev/fs/localimport/__init__.py |
Internal CLI command (hidden) used inside dev environments to perform the actual file import |
tests/env/dev/test_fs.py |
Comprehensive unit tests for the file operation logic in fs.py |
tests/env/dev/test_interface.py |
Updated interface test stub to include new methods |
tests/env/dev/types/test_linux_container.py |
Updated tests for shared directory path changes and added tests for export/import orchestration |
tests/env/dev/fixtures/fs_tests/* |
Test fixture files for verifying file operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| status = env.status() | ||
|
|
||
| possible_states = {EnvironmentState.STARTED} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be possible to have more states in the future? Otherwise you can maybe simplify the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, similar to the same block in export, this might end up being different for every environment type. We could maybe imagine modifying a VM's filesystem even when it is stopped by directly editing the underlying volumes for example.
| status = env.status() | ||
|
|
||
| # TODO: This might end up depending on the environment type. | ||
| # For `linux-container` though, `docker cp` also works on stopped containers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this comment mean the command only works partially (ie just for linux-container environment)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well for the moment that's the only environment type we have, so yes it's only implemented for linux-container as of yet. But I've added abstract methods in the interface definition so every type we implement in the feature will need to implement the required methods as well.
ofek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
| @@ -0,0 +1,73 @@ | |||
| # SPDX-FileCopyrightText: 2024-present Datadog, Inc. <[email protected]> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure the years match when new files were introduced.
| mkpath: bool, | ||
| ) -> None: | ||
| """ | ||
| Internal command used to call import_from_dir in dev envs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this specific function required when we can make any subprocess call within environments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this ; discussed offline - just use cp -r
| from dda.utils.fs import temp_directory | ||
|
|
||
| # 1. Create a temporary directory on the host filesystem | ||
| with temp_directory() as wd: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should use the shared directory for communication in all circumstances just like in the import_files method and then have the host move the staged source to the destination. The only situation in which this wouldn't be as desirable would be copying many small files but the main use case here is large build artifacts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested offline: using the shared dir is way faster (in both directions) than docker cp (!), at least for single large files (since we'll mostly be exfiltrating build artifacts this is the most common usecase)
-> Use the shared dir for both methods
| @dynamic_command(short_help="""Import files and directories into a developer environment""") | ||
| @option_env_type() | ||
| @click.option("--id", "instance", default="default", help="Unique identifier for the environment") | ||
| @click.argument("sources", nargs=-1, required=True, type=click.Path(exists=True, resolve_path=True, path_type=Path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only allow one source: globs don't work so it would be weird
| dest.unlink() | ||
|
|
||
|
|
||
| def import_from_dir(source_dir: Path, destination_spec: Path, *, recursive: bool, force: bool, mkpath: bool) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably remove this entire file, as we will just rely on the semantics of cp and mv - note cp does overwrite files if you ask it to.
| raise NotImplementedError | ||
|
|
||
| @abstractmethod | ||
| def import_files( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep this abstraction though, it's good
Create two commands,
dda env dev fs importanddda env dev fs exportthat allow for easily moving files and directories in and out of dev envs.This is useful as an addition to standard bind mounts, because people might not think of bind-mounting something into the container upon creation. Such a feature was heavily requested by our users.
The feature was implemented by making the necessary changes to the dev env interface to make sure that such a feature will also be supported by other dev env types (remote VMs...). As much logic as possible was made agnostic to the type of dev env in question (everything in
src/dda/env/dev/fs.py)