-
Notifications
You must be signed in to change notification settings - Fork 1
Refactor DBC file handling and path resolution #11
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
Conversation
Centralizes DBC file location logic in both file-uploader and startup-data-loader, allowing override via DBC_FILE_PATH environment variable and supporting shared volume mounting. Updates Docker Compose and documentation to reflect new DBC file sharing approach, removes redundant DBC copy from startup-data-loader image, and renames example.dbc to repository root for consistency.
|
Summary
Testing
|
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 refactors DBC file handling to centralize path resolution logic and enable sharing a single DBC file across multiple containers via volume mounts. The key architectural change is moving from bundled DBC files in each container image to a shared volume-mounted approach.
Key Changes:
- Introduces
_resolve_dbc_path()function in bothstartup-data-loaderandfile-uploaderwith configurable fallback chain (env override → shared volume → local fallback) - Moves
example.dbcfrominstaller/startup-data-loader/toinstaller/(repository root) - Updates Docker Compose to mount the DBC file as a shared read-only volume and adds
DBC_FILE_PATHenvironment variable support
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| installer/startup-data-loader/load_data.py | Adds DBC path resolution function and constants; replaces directory scan with centralized lookup |
| installer/startup-data-loader/example.dbc | Removes DBC file from service-specific directory (moved to repository root) |
| installer/startup-data-loader/README.md | Updates documentation to reflect shared DBC file location and configuration |
| installer/startup-data-loader/Dockerfile | Removes COPY *.dbc directive since DBC is now mounted via volume |
| installer/file-uploader/helper.py | Adds DBC path resolution function with same logic as startup-data-loader |
| installer/example.dbc | Adds shared DBC file at repository root |
| installer/docker-compose.yml | Configures volume mounts and DBC_FILE_PATH environment variable for both services |
| installer/.env.example | Adds DBC_FILE_PATH configuration with default value |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
installer/docker-compose.yml
Outdated
| INFLUXDB_URL: "${INFLUXDB_URL:-http://influxdb3:8181}" | ||
| BACKFILL: "1" | ||
| CSV_RESTART_INTERVAL: "${CSV_RESTART_INTERVAL:-10}" | ||
| DBC_FILE_PATH: "/installer/${DBC_FILE_PATH:-example.dbc}" |
Copilot
AI
Nov 14, 2025
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.
The environment variable DBC_FILE_PATH is set to /installer/${DBC_FILE_PATH:-example.dbc}, which creates a circular reference. If DBC_FILE_PATH is set to a custom value like custom.dbc, this will expand to /installer/custom.dbc inside the container. However, the volume mount on line 164 will mount the file from ./${DBC_FILE_PATH:-example.dbc} (i.e., ./custom.dbc) to /installer/${DBC_FILE_PATH:-example.dbc} (i.e., /installer/custom.dbc).
This creates a mismatch: if someone sets DBC_FILE_PATH=my_files/custom.dbc, the environment variable becomes /installer/my_files/custom.dbc, but the volume mount would try to mount ./my_files/custom.dbc to /installer/my_files/custom.dbc, which likely isn't the intended behavior.
The environment variable should simply be set to the container path where the file is mounted: DBC_FILE_PATH: "/installer/example.dbc" (without the nested substitution).
| DBC_FILE_PATH: "/installer/${DBC_FILE_PATH:-example.dbc}" | |
| DBC_FILE_PATH: "/installer/example.dbc" |
installer/docker-compose.yml
Outdated
| DBC_FILE_PATH: "/installer/${DBC_FILE_PATH:-example.dbc}" | ||
| volumes: | ||
| - ./${DBC_FILE_PATH:-example.dbc}:/installer/${DBC_FILE_PATH:-example.dbc}:ro |
Copilot
AI
Nov 14, 2025
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.
The volume mount pattern ./${DBC_FILE_PATH:-example.dbc}:/installer/${DBC_FILE_PATH:-example.dbc}:ro has a problematic design. If DBC_FILE_PATH contains a path like subdir/file.dbc, this will try to mount ./subdir/file.dbc to /installer/subdir/file.dbc. However, the target directory /installer/subdir/ may not exist in the container, causing the mount to fail.
Additionally, this creates unnecessary coupling between the host path structure and the container path structure. A simpler approach would be to always mount to a fixed location like /installer/example.dbc and use DBC_FILE_PATH only on the host side:
- ./${DBC_FILE_PATH:-example.dbc}:/installer/example.dbc:roThen set the environment variable to the fixed container path:
DBC_FILE_PATH: "/installer/example.dbc"| DBC_FILE_PATH: "/installer/${DBC_FILE_PATH:-example.dbc}" | |
| volumes: | |
| - ./${DBC_FILE_PATH:-example.dbc}:/installer/${DBC_FILE_PATH:-example.dbc}:ro | |
| DBC_FILE_PATH: "/installer/example.dbc" | |
| volumes: | |
| - ./${DBC_FILE_PATH:-example.dbc}:/installer/example.dbc:ro |
installer/docker-compose.yml
Outdated
| - "8084:8084" | ||
| volumes: | ||
| - ./file-uploader:/app | ||
| - ./${DBC_FILE_PATH:-example.dbc}:/installer/${DBC_FILE_PATH:-example.dbc}:ro |
Copilot
AI
Nov 14, 2025
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.
Same issue as line 164: the volume mount pattern ./${DBC_FILE_PATH:-example.dbc}:/installer/${DBC_FILE_PATH:-example.dbc}:ro is problematic. If DBC_FILE_PATH contains a path like subdir/file.dbc, this will try to mount to /installer/subdir/file.dbc, but the target directory may not exist.
Consider simplifying to a fixed container path:
- ./${DBC_FILE_PATH:-example.dbc}:/installer/example.dbc:ro| - ./${DBC_FILE_PATH:-example.dbc}:/installer/${DBC_FILE_PATH:-example.dbc}:ro | |
| - ./${DBC_FILE_PATH:-example.dbc}:/installer/example.dbc:ro |
installer/docker-compose.yml
Outdated
| - INFLUXDB_TOKEN=${INFLUXDB_ADMIN_TOKEN:-apiv3_dev-influxdb-admin-token} | ||
| - INFLUXDB_URL=${INFLUXDB_URL:-http://influxdb3:8181} | ||
| - FILE_UPLOADER_WEBHOOK_URL=${FILE_UPLOADER_WEBHOOK_URL:-} | ||
| - DBC_FILE_PATH=/installer/${DBC_FILE_PATH:-example.dbc} |
Copilot
AI
Nov 14, 2025
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.
Same circular reference issue as line 162. The environment variable DBC_FILE_PATH should be set to a fixed container path like /installer/example.dbc instead of /installer/${DBC_FILE_PATH:-example.dbc}.
| def _resolve_dbc_path() -> Path: | ||
| """Resolve the DBC path using env override, shared installer copy or local fallback.""" | ||
| env_override = os.getenv(DBC_ENV_VAR) | ||
| if env_override: | ||
| env_path = Path(env_override).expanduser() | ||
| if env_path.exists(): | ||
| return env_path | ||
| print(f"⚠️ {DBC_ENV_VAR}={env_override} not found; falling back to default lookup.") | ||
|
|
||
| shared_candidates = [ | ||
| INSTALLER_ROOT / DBC_FILENAME, | ||
| Path("/installer") / DBC_FILENAME, | ||
| ] | ||
| for candidate in shared_candidates: | ||
| if candidate.exists(): | ||
| return candidate | ||
|
|
||
| # Final fallback: look for local .dbc files (maintains backwards compatibility) | ||
| current_dir = Path(__file__).resolve().parent | ||
| dbc_candidates = sorted( | ||
| current_dir.glob("*.dbc"), | ||
| key=lambda file_path: file_path.stat().st_mtime, | ||
| reverse=True | ||
| ) | ||
| if dbc_candidates: | ||
| return dbc_candidates[0] | ||
|
|
||
| raise FileNotFoundError( | ||
| f"Could not locate {DBC_FILENAME}. Place it in the installer root " | ||
| f"or set {DBC_ENV_VAR} to the desired path." | ||
| ) | ||
|
|
Copilot
AI
Nov 14, 2025
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.
The _resolve_dbc_path() function is duplicated between load_data.py and helper.py with nearly identical logic. This violates the DRY (Don't Repeat Yourself) principle and makes maintenance harder—any bug fix or enhancement needs to be applied in two places.
Consider extracting this shared logic into a common utility module that both files can import, or if the files are always used together, consolidate the function in one location and import it from the other.
|
|
||
| raise FileNotFoundError( | ||
| f"Could not locate {DBC_FILENAME}. Place it in the installer root " | ||
| f"or set {DBC_ENV_VAR} to the desired path." |
Copilot
AI
Nov 14, 2025
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.
The error messages in the two _resolve_dbc_path() functions are slightly inconsistent:
load_data.py(line 204): "Place it in the installer root"helper.py(line 73): "Place it in the repository root"
Both refer to the same location (INSTALLER_ROOT), so the terminology should be consistent. Consider using "installer root" in both places or clarifying that they mean the same directory.
| f"or set {DBC_ENV_VAR} to the desired path." | |
| f"(INSTALLER_ROOT) or set {DBC_ENV_VAR} to the desired path." |
| DBC_ENV_VAR = "DBC_FILE_PATH" | ||
| DBC_FILENAME = "example.dbc" | ||
| INSTALLER_ROOT = Path(__file__).resolve().parent.parent |
Copilot
AI
Nov 14, 2025
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.
The constants DBC_ENV_VAR, DBC_FILENAME, and INSTALLER_ROOT are duplicated between load_data.py and helper.py. Like the _resolve_dbc_path() function, these should be defined in a shared location to avoid potential inconsistencies and maintenance issues.
If these constants ever need to be updated (e.g., changing the default filename or environment variable name), the change would need to be made in multiple files.
| DBC_ENV_VAR = "DBC_FILE_PATH" | |
| DBC_FILENAME = "example.dbc" | |
| INSTALLER_ROOT = Path(__file__).resolve().parent.parent | |
| from .constants import DBC_ENV_VAR, DBC_FILENAME, INSTALLER_ROOT |
Updated docker-compose.yml to use a fixed container path (/installer/example.dbc) for the DBC file, while allowing the host file name to remain flexible. This change simplifies environment variable usage and volume mounting for startup-data-loader and file-uploader services.
Centralizes DBC file location logic in both file-uploader and startup-data-loader, allowing override via DBC_FILE_PATH environment variable and supporting shared volume mounting. Updates Docker Compose and documentation to reflect new DBC file sharing approach, removes redundant DBC copy from startup-data-loader image, and renames example.dbc to repository root for consistency.