Refactor dependencies and improve Docker integration - #4
Conversation
pannoury
commented
Feb 14, 2026
pannoury
commented
- Updated dependencies in pyproject.toml and requirements.txt to specific versions for better compatibility.
- Enhanced the init command to handle reference manifest files more effectively, including downloading from storage and updating variables.
- Introduced a logging utility for better error handling and exception reporting.
- Improved storage connector configuration by adding a name field for better identification.
- Added functions to parse Docker environment variables and volume mappings, allowing for better integration with Docker runners.
- Updated the runner logic to handle absolute paths for local and Docker runners, ensuring correct path resolution.
- Modified the Variables class to skip reference manifest validation if a state URI is provided, improving initialization efficiency.
- Updated dependencies in pyproject.toml and requirements.txt to specific versions for better compatibility. - Enhanced the init command to handle reference manifest files more effectively, including downloading from storage and updating variables. - Introduced a logging utility for better error handling and exception reporting. - Improved storage connector configuration by adding a name field for better identification. - Added functions to parse Docker environment variables and volume mappings, allowing for better integration with Docker runners. - Updated the runner logic to handle absolute paths for local and Docker runners, ensuring correct path resolution. - Modified the Variables class to skip reference manifest validation if a state URI is provided, improving initialization efficiency.
There was a problem hiding this comment.
Pull request overview
This pull request refactors dependencies to use pinned versions and enhances Docker integration capabilities. The changes improve state management by allowing reference manifests to be downloaded from storage URIs, add Docker-specific path resolution for container environments, and introduce better error logging utilities.
Changes:
- Pinned all dependencies to specific versions in pyproject.toml, requirements.txt, and Pipfile for better reproducibility
- Added Docker environment variable and volume parsing functions to support container path resolution
- Modified Variables class to skip reference manifest validation when a state_uri is provided
- Enhanced init command to download manifests from storage and update path references dynamically
- Added print_exception utility for detailed error logging with file and line information
- Added name field to StorageConnectorConfig for better identification in logs
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Pinned dependency versions from ranges to exact versions |
| requirements.txt | Complete dependency lock with specific versions for all transitive dependencies |
| Pipfile | Updated to use exact version pinning instead of version ranges |
| Pipfile.lock | Regenerated lock file with new pinned versions |
| src/variables/init.py | Added conditional logic to skip reference manifest validation when state_uri is provided |
| src/schema.py | Added name field to StorageConnectorConfig TypedDict |
| src/runners/docker.py | Added parse_docker_env, parse_docker_volumes, and get_container_paths functions |
| src/runners/init.py | Enhanced resolve_dbt_commands to use container paths for Docker runner |
| src/commands/init.py | Modified to download manifests from storage and update variables with local paths |
| src/logging.py | Added print_exception utility function for enhanced error reporting |
| src/connectors/init.py | Added name values to storage connector configurations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if dbtstate_dir is None: | ||
| logger.error("No valid path found for downloading manifest file. Please specify a valid --state path or ensure your dbt_project_dir is correct.") | ||
| sys.exit(1) |
There was a problem hiding this comment.
The condition on line 149 checks variables.runner == "docker" or variables.reference_state is None, then line 153 checks if dbtstate_dir is None. However, dbtstate_dir is assigned in both branches of the if-else (lines 150 and 152), so it will never be None at line 153. This makes the error handling on lines 153-155 unreachable. Consider removing this dead code or restructuring the logic if there are other paths where dbtstate_dir could be None.
| def parse_docker_env(runner_config: RunnerConfig) -> dict: | ||
| """Parse docker_env list into a dictionary.""" | ||
| env_dict = {} | ||
| docker_env = runner_config.get("docker_env", []) | ||
| if docker_env: | ||
| for env in docker_env: | ||
| if "=" in env: | ||
| key, value = env.split("=", 1) | ||
| env_dict[key] = value | ||
| return env_dict | ||
|
|
||
| def parse_docker_volumes(runner_config: RunnerConfig) -> dict: | ||
| """Parse docker_volumes list into a host_path -> container_path mapping.""" | ||
| volume_map = {} | ||
| docker_volumes = runner_config.get("docker_volumes", []) | ||
| for volume in docker_volumes: | ||
| parts = volume.split(":", 2) | ||
| if len(parts) >= 2: | ||
| host_path = parts[0] | ||
| container_path = parts[1] | ||
| volume_map[host_path] = container_path | ||
| return volume_map | ||
|
|
||
| def get_container_paths(runner_config: RunnerConfig) -> dict: | ||
| """Get container paths for dbt configuration variables. | ||
|
|
||
| Returns a mapping of variable names to their container paths: | ||
| { | ||
| 'dbt_project_dir': '/dbt', | ||
| 'profiles_dir': '/dbt', | ||
| 'reference_state': '/dbt/.dbtstate' | ||
| } | ||
| """ | ||
| env_dict = parse_docker_env(runner_config) | ||
| volume_map = parse_docker_volumes(runner_config) | ||
|
|
||
| container_path_map = {} | ||
|
|
||
| # For dbt_project_dir: use DBT_PROJECT_DIR env or derive from volume mapping | ||
| if "DBT_PROJECT_DIR" in env_dict: | ||
| container_path_map["dbt_project_dir"] = env_dict["DBT_PROJECT_DIR"] | ||
| else: | ||
| # Try to derive from volume mapping | ||
| dbt_project_host = runner_config.get("dbt_project_dir") | ||
| if dbt_project_host and dbt_project_host in volume_map: | ||
| container_path_map["dbt_project_dir"] = volume_map[dbt_project_host] | ||
|
|
||
| # For profiles_dir: use DBT_PROFILES_DIR env | ||
| if "DBT_PROFILES_DIR" in env_dict: | ||
| container_path_map["profiles_dir"] = env_dict["DBT_PROFILES_DIR"] | ||
|
|
||
| # For reference_state: use DBT_STATE env | ||
| if "DBT_STATE" in env_dict: | ||
| container_path_map["reference_state"] = env_dict["DBT_STATE"] | ||
|
|
||
| return container_path_map |
There was a problem hiding this comment.
The new Docker parsing functions (parse_docker_env, parse_docker_volumes, get_container_paths) lack test coverage. While tests exist for get_docker_env and get_docker_volumes in tests/unit/runners/test_docker.py, the new parsing functions should have dedicated tests to ensure they handle edge cases correctly (empty strings in env vars, malformed volume strings, etc.).
| for env in docker_env: | ||
| if "=" in env: | ||
| key, value = env.split("=", 1) | ||
| env_dict[key] = value | ||
| return env_dict |
There was a problem hiding this comment.
The function silently skips environment variables that don't contain an equals sign. This could lead to configuration errors being silently ignored. Consider either logging a warning for malformed entries or documenting that entries without '=' will be skipped. The same applies to parse_docker_volumes for malformed volume specifications.
| logger.error(f"File: {e.__traceback__.tb_frame.f_code.co_filename}") | ||
| logger.error(f"Line: {e.__traceback__.tb_lineno}") No newline at end of file |
There was a problem hiding this comment.
The exception traceback may be None if the exception hasn't been raised yet or has been re-raised without a traceback. Accessing __traceback__.tb_frame without checking if __traceback__ is None will cause an AttributeError. Add a null check before accessing traceback attributes.
| logger.error(f"File: {e.__traceback__.tb_frame.f_code.co_filename}") | |
| logger.error(f"Line: {e.__traceback__.tb_lineno}") | |
| tb = e.__traceback__ | |
| if not tb: | |
| logger.error("Traceback information is not available.") | |
| return | |
| logger.error(f"File: {tb.tb_frame.f_code.co_filename}") | |
| logger.error(f"Line: {tb.tb_lineno}") |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/commands/init.py:20
append_dbt_variables_to_commandis imported but no longer used in this module after switching toresolve_dbt_commands. Removing the unused import will prevent lint/type-check noise and keep the module tidy.
from src.variables import Variables
from src.cache import CacheManager
from src.runners import resolve_dbt_commands, run_dbt_command, append_dbt_variables_to_command
from src.utilities.graph_utils import (
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_docker_env(runner_config: RunnerConfig) -> dict | None: | ||
| """Build Docker environment variables based on runner configuration.""" | ||
| # Don't automatically set environment variables - let the command-line flags handle paths | ||
| # and only use user-provided docker_env | ||
| env_dict = {} | ||
|
|
||
| # Only use user-provided docker_env | ||
| user_env = parse_docker_env(runner_config) | ||
| env_dict.update(user_env) | ||
|
|
||
| return env_dict |
There was a problem hiding this comment.
get_docker_env is annotated to return dict | None, but it always returns a dict (even when empty). Updating the return annotation to just dict (and possibly the docstring) would better reflect the actual behavior and simplify callers.
| def print_exception( | ||
| e: Exception, | ||
| base_message: str = "Unexpected error", | ||
| ) -> None: | ||
| """Print an exception with file and line number information.""" | ||
| logger.error(f"{base_message}: {e}") | ||
| logger.error(f"File: {e.__traceback__.tb_frame.f_code.co_filename}") | ||
| logger.error(f"Line: {e.__traceback__.tb_lineno}") No newline at end of file |
There was a problem hiding this comment.
print_exception dereferences e.__traceback__ without a guard and only logs a single file/line, which can lose context (and can fail if __traceback__ is unexpectedly None). Consider using logger.exception(...) (or traceback.format_exception) to reliably include the full stack trace, and fall back gracefully when traceback info isn’t present.
| # Try to derive from volume mapping | ||
| dbt_project_host = runner_config.get("dbt_project_dir") | ||
| if dbt_project_host and dbt_project_host in volume_map: | ||
| container_path_map["dbt_project_dir"] = volume_map[dbt_project_host] |
There was a problem hiding this comment.
get_container_paths tries to derive dbt_project_dir from the volume map via dbt_project_host in volume_map, but volume_map keys are not normalized (and may be relative) while dbt_project_dir may be absolute. This makes derivation fail for common cases like ./dbt:/dbt. Consider normalizing both sides (e.g., store absolute host paths in parse_docker_volumes, or compare with get_absolute_path for both).
…l, and migration commands
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 21 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Skip reference manifest validation if state_uri is provided (will be downloaded later) | ||
| if self.state_uri is not None: | ||
| self._resolved["reference_manifest_file"] = None | ||
| else: | ||
| self._resolved["reference_manifest_file"] = get_reference_manifest_file(self.reference_state) |
There was a problem hiding this comment.
When state_uri is provided, this branch sets reference_manifest_file to None, but reference_state is still marked required earlier in Variables resolution. As a result, commands like dbt-ci init --state-uri ... (documented in main.py) will exit before reaching this code unless --reference-state/--state is also provided. Consider making reference_state conditionally optional when state_uri is set (or synthesizing a default download directory before required-field validation).
| @@ -164,9 +168,9 @@ def run_with_mode( | |||
| ) | |||
There was a problem hiding this comment.
run_with_mode() still builds dbt commands via append_dbt_variables_to_command, which doesn’t apply the new runner-specific path resolution (absolute host paths for local/dbt, container paths for docker, and --state). This will break Docker runner path resolution (host paths passed into the container) and diverges from init’s updated behavior. Use resolve_dbt_commands() here as well (or ensure equivalent path mapping is applied).
| import click | ||
| from google.cloud import bigquery | ||
| from src.schema import DeleteMapNode, EphemeralMapNode, MigrationMap | ||
| from src.utilities.paths import get_profile, get_profiles_file |
There was a problem hiding this comment.
get_profile is imported but no longer used in this module. Removing the unused import will avoid confusion and keeps linting clean.
| from src.utilities.paths import get_profile, get_profiles_file | |
| from src.utilities.paths import get_profiles_file |
| # Install dependencies excluding dbt-core, then install specific dbt-core version | ||
| RUN grep -v "^dbt-core" requirements.txt > /tmp/requirements.txt && \ |
There was a problem hiding this comment.
This Dockerfile installs a fully pinned requirements.txt (including dbt-* internal packages) and then swaps only dbt-core via DBT_CORE_VERSION. When building images for other dbt versions (see workflow matrix), the pinned dbt-* dependency set will likely be incompatible with the chosen dbt-core version. Consider either (a) installing only dbt-core==${DBT_CORE_VERSION} (+ the appropriate adapter) and letting it resolve compatible dbt-* deps, or (b) maintaining per-dbt-version constraints/lockfiles and selecting the correct one at build time.
| # Install dependencies excluding dbt-core, then install specific dbt-core version | |
| RUN grep -v "^dbt-core" requirements.txt > /tmp/requirements.txt && \ | |
| # Install dependencies excluding all dbt-* packages, then install specific dbt-core version | |
| RUN grep -v "^dbt-" requirements.txt > /tmp/requirements.txt && \ |
| # Define dbt-core version range | ||
| # Update these versions as new releases become available | ||
| dbt_version: | ||
| - '1.8.0' | ||
| - '1.8.7' | ||
| - '1.9.0' | ||
| - '1.9.1' | ||
| - '1.10.0' | ||
| - '1.10.13' | ||
| - '1.11.0' | ||
| - '1.11.5' | ||
|
|
There was a problem hiding this comment.
The matrix builds images across multiple dbt_version values, but the build context uses a single pinned requirements.txt and only varies dbt-core via build arg. Unless the non-core dbt packages are also varied, this matrix is likely to fail or produce inconsistent images for versions other than the one the pins were generated for. Consider generating constraints per dbt version, or simplifying the install to dbt-core==${{ matrix.dbt_version }} (+ adapter) so dependencies stay consistent.
