fix(bundle): use --source-code-path for apps deploy - #50
Conversation
Databricks CLI v0.295.0+ removed --target/--var from `apps deploy`. DS-PATCH-038 resolves the workspace source path from `bundle summary`, auto-generates an `app.yml` with env entries resolved against `value_from` references and bundle variables, and invokes `apps deploy <name> --source-code-path <path>`. Closes #39. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 103 complexity · 0 duplication
Metric Results Complexity 103 Duplication 0
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
The PR successfully implements the transition to --source-code-path for Databricks CLI v0.295.0+, but it is not currently up to standards. While the functional logic for app.yml generation is present, the script scripts/deploy_databricks_app.py has reached a high level of cyclomatic complexity (CCN 29) without adequate test coverage. A major discrepancy was found: the documentation updates (docs/deployment_guide.md and the Traceability Matrix v1.6) required by the acceptance criteria are completely missing from the PR. Furthermore, a high-severity type-safety issue and a medium-severity logic flaw in command string handling must be addressed before merging.
About this PR
- The required updates to docs/deployment_guide.md and the Traceability Matrix (v1.6) are missing from this change set. These are necessary to fulfill the PR requirements.
- The deployment script has significantly increased in complexity without a corresponding increase in automated test coverage, posing a high risk for future regressions.
Test suggestions
- Verify
databricks apps deploycommand construction omits bundle flags and includes source-code-path. - Verify
app.ymlgeneration resolves Volume paths from concrete securable names. - Verify
app.ymlgeneration falls back to bundle variables for Volume path resolution when securable names contain references. - Verify
app.ymlgeneration resolves Job IDs from resource references in the bundle summary. - Verify
app.ymlgeneration preserves literal environment variable values. - Verify
app.ymlgeneration returns None (skipping upload) when no command is specified in the app config. - Verify unrecognized resource references resolve to an empty string in environment variables (fail-soft).
- Increase unit test coverage for complex resource resolution logic in
scripts/deploy_databricks_app.pyto mitigate complexity risks.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Increase unit test coverage for complex resource resolution logic in `scripts/deploy_databricks_app.py` to mitigate complexity risks.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| return 0 | ||
|
|
||
| deploy_cmd = ["databricks", "apps", "deploy", *bundle_args] | ||
| app_yaml = _build_app_yaml_content(summary, args.app_key) |
There was a problem hiding this comment.
🔴 HIGH RISK
The summary variable needs to be explicitly narrowed to a dictionary type (e.g., using an assertion) to satisfy type checkers and improve safety before it is passed to _build_app_yaml_content.
| return ["-p", profile] if profile else [] | ||
|
|
||
|
|
||
| def _resolve_app_resource_values(summary: dict[str, Any], app_key: str) -> dict[str, str]: |
There was a problem hiding this comment.
🟡 MEDIUM RISK
This function is becoming difficult to maintain due to its high cyclomatic complexity. Consider refactoring the resolution logic for Volumes and Jobs into dedicated helper functions like _resolve_volume_resource and _resolve_job_resource.
| env_out.append({"name": name, "value": ""}) | ||
|
|
||
| return yaml.safe_dump( | ||
| {"command": list(command), "env": env_out}, |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Ensure 'command' is handled correctly if it arrives as a string to avoid character-level splitting in the generated app.yml.
| {"command": list(command), "env": env_out}, | |
| {"command": [command] if isinstance(command, str) else list(command), "env": env_out}, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75e71422d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| apps = summary.get("resources", {}).get("apps", {}) or {} | ||
| app = apps.get(app_key) or {} | ||
| config = app.get("config") or {} | ||
| app_resources = config.get("resources") or [] |
There was a problem hiding this comment.
Read app resource bindings from top-level app node
This resolver pulls bindings from config.resources, but the bundle app definition keeps resources at the app root (see resources/driftsentinel_app.yml, where resources is a sibling of config). If bundle summary preserves that schema, app_resources is always empty here, so _build_app_yaml_content resolves every value_from entry (including RUNTIME_VOLUME_PATH and DATASET_PIPELINE_JOB_ID) to "", yielding an app.yml that deploys with missing runtime wiring.
Useful? React with 👍 / 👎.
aa94cc5
into
main
Summary
--target/--varflags ondatabricks apps deploywithapps deploy <name> --source-code-path <path>app.ymlat the workspace source-code-path withcommand:and env entries resolved againstvalue_fromresource references and bundle variablesspecs/DS-PATCH-038_app_deploy_source_code_path.md; traceability matrix bumped to v1.6README.mdanddocs/deployment_guide.mdCloses #39.
Test plan
make lint(uv run ruff check .) — cleanmake typecheck(uv run mypy src/driftsentinel tests) — 60 source files, no issuesmake test— 503 passedtests/test_app.py::TestDeployScriptCommandShape,TestDeployScriptAppYamlGeneration,TestDeployScriptResourceResolutioncover command shape, env resolution, and resource lookup edge casesmake app-deploy CATALOG=<C> PROFILE=<P>reachesapps getstate=RUNNINGandactive_deployment.status.state=SUCCEEDED(post-merge, per DS-PATCH-038 §6.3)🤖 Generated with Claude Code