-
Notifications
You must be signed in to change notification settings - Fork 193
Add docker-test skill for running commands in arcticdb Docker container #2947
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: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,106 @@ | ||||||
| --- | ||||||
| name: docker-test | ||||||
| description: Run commands or tests inside the arcticdb Docker container | ||||||
| argument-hint: "[command]" | ||||||
| --- | ||||||
|
|
||||||
| Run commands or tests inside the arcticdb Docker container. | ||||||
|
|
||||||
| ## Usage | ||||||
|
|
||||||
| - `/docker-test` — start the container if not running, then open an interactive prompt for what to run | ||||||
| - `/docker-test pytest python/tests/unit/some_test.py` — run a specific test | ||||||
| - `/docker-test python -c "import arcticdb"` — run arbitrary command | ||||||
| - `/docker-test --name mydev pytest ...` — use a named container `arc-dev-mydev` (reusable across sessions) | ||||||
| - `/docker-test --name mydev` — reconnect to an existing named container | ||||||
|
|
||||||
| ## Instructions | ||||||
|
|
||||||
| Image name: `arcticdb-env` | ||||||
| Dockerfile: `docker/Dockerfile` | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually subtly different from the docker images we use on the CI. For C++ test I think we use Dockerfile_clang and for python wheel we use the bespoke build_manylinux_image.sh. It might be useful to add some description of this and information of how the ghcr images are called so when you tell claude "Reproduce this failure on the CI within the docker image" it can figure out which image it needs to pull |
||||||
|
|
||||||
| ### 0. Determine the container name | ||||||
|
|
||||||
| First, check if `$ARGUMENTS` starts with `--name <name>`. If so: | ||||||
| - Use `arc-dev-<name>` as the container name. | ||||||
| - Strip `--name <name>` from `$ARGUMENTS` before executing in step 3. | ||||||
|
|
||||||
| If no `--name` was provided, generate a random name on **first invocation only**: | ||||||
| ```bash | ||||||
| _ARC_CONTAINER="arc-dev-$(head -c4 /dev/urandom | xxd -p)" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
_ARC_CONTAINER="arc-dev-$(head -c4 /dev/urandom | xxd -p)"
Suggested change
or, if _ARC_CONTAINER="arc-dev-$(od -A n -t x4 -N 4 /dev/urandom | tr -d ' \n')"
|
||||||
| ``` | ||||||
|
|
||||||
| **Important**: Once determined, reuse the same container name for all subsequent `/docker-test` invocations in this conversation. Do NOT re-derive. | ||||||
|
|
||||||
| Named containers (`--name`) survive across sessions so the user can reconnect later. Random containers are ephemeral. | ||||||
|
|
||||||
| ### 1. Ensure the image is built | ||||||
|
|
||||||
| Check if the image exists: | ||||||
| ```bash | ||||||
| docker image inspect arcticdb-env >/dev/null 2>&1 | ||||||
| ``` | ||||||
|
|
||||||
| If it doesn't exist, build it from the repo root: | ||||||
| ```bash | ||||||
| docker build -f docker/Dockerfile -t arcticdb-env . | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dockerfile path has diverged from the actual repo structure The skill says: But the actual file in the repo is docker build -f docker/Dockerfile -t arcticdb-env .Building from the repo root ( |
||||||
| ``` | ||||||
|
|
||||||
| ### 2. Ensure the container is running | ||||||
|
|
||||||
| Check the container state with a single command: | ||||||
| ```bash | ||||||
| docker inspect --format '{{.State.Status}}' "$_ARC_CONTAINER" 2>/dev/null | ||||||
| ``` | ||||||
|
|
||||||
| - If the command **fails** (container does not exist), create it: | ||||||
| ```bash | ||||||
| docker run -d --name "$_ARC_CONTAINER" \ | ||||||
| -v "$(git rev-parse --show-toplevel)":/workspace \ | ||||||
| -w /workspace \ | ||||||
| arcticdb-env sleep infinity | ||||||
| ``` | ||||||
|
|
||||||
| - If it returns **`exited`** or **`created`**, start it: | ||||||
| ```bash | ||||||
| docker start "$_ARC_CONTAINER" | ||||||
| ``` | ||||||
|
|
||||||
| - If it returns **`running`**, nothing to do. | ||||||
|
|
||||||
| ### 3. Execute the command | ||||||
|
|
||||||
| Run the user's command inside the container. For simple commands pass them directly: | ||||||
| ```bash | ||||||
| docker exec -w /workspace "$_ARC_CONTAINER" $ARGUMENTS | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unquoted docker exec -w /workspace "$_ARC_CONTAINER" $ARGUMENTSThe previous commit correctly wrapped shell-operator cases in The simplest fix is to always use
Suggested change
If the |
||||||
| ``` | ||||||
|
|
||||||
| If the command uses shell operators (pipes `|`, redirects `>`, `&&`, etc.), wrap it with `bash -c`: | ||||||
| ```bash | ||||||
| docker exec -w /workspace "$_ARC_CONTAINER" bash -c "$ARGUMENTS" | ||||||
| ``` | ||||||
| When using `bash -c`, escape any inner quotes appropriately. | ||||||
|
|
||||||
| If no command was provided, ask the user what they want to run. | ||||||
|
|
||||||
| ### 4. Show results | ||||||
|
|
||||||
| Display the command output. If the command fails, help debug as usual. | ||||||
|
|
||||||
| ### 5. Cleanup | ||||||
|
|
||||||
| When the user explicitly asks to clean up, remove the current session's container: | ||||||
| ```bash | ||||||
| docker rm -f "$_ARC_CONTAINER" | ||||||
| ``` | ||||||
|
|
||||||
| To list all arc-dev containers from this skill: | ||||||
| ```bash | ||||||
| docker ps -a --filter "name=^arc-dev-" --format "table {{.Names}}\t{{.Status}}" | ||||||
| ``` | ||||||
|
|
||||||
| To prune all **stopped** arc-dev containers (only do this if the user asks): | ||||||
| ```bash | ||||||
| docker container ls -a --filter "name=^arc-dev-" --filter "status=exited" -q | xargs -r docker rm | ||||||
| ``` | ||||||
| Warn the user before pruning, as this will remove named containers they may want to reuse. | ||||||
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.
No mention of what the Docker image actually provides / prerequisites
The Usage section jumps straight to example invocations without explaining what environment the container provides (Python version, whether ArcticDB is pre-installed or needs to be built, etc.). A developer encountering this skill for the first time won't know whether they need to build ArcticDB into the image first or whether
import arcticdbwill work out of the box.Adding a brief note like "The image is a build environment — ArcticDB must be installed into it (e.g. by running
pip install -ve .inside the container) before running tests" would significantly improve usability.