Repository navigation
Add lifecycle.depends_on to order resources without references - #7120
Merged
Merged
Conversation
Contributor
|
Commit: Regression test reportSelected 4 changed tests (limit=20, 0 not selected) 7 tests: regression=7 unreleased=0 coverage=0 failing=0
TestAccept/bundle/lifecycle/depends-on-validation/DMS=: branch ✅ | main (4d88efe) ❌ | latest (v1.22.1) ❌main (4d88efe): latest (v1.22.1): same output as main (4d88efe) TestAccept/bundle/lifecycle/depends-on-validation/DMS=true: branch ✅ | main (4d88efe) ❌ | latest (v1.22.1) ❌main (4d88efe): latest (v1.22.1): same output as main (4d88efe) TestAccept/bundle/lifecycle/depends-on/DMS=: branch ✅ | main (4d88efe) ❌ | latest (v1.22.1) ❌main (4d88efe): latest (v1.22.1): TestAccept/bundle/lifecycle/depends-on/DMS=true: branch ✅ | main (4d88efe) ❌ | latest (v1.22.1) ❌main (4d88efe): latest (v1.22.1): TestAccept/bundle/migrate/auto/depends-on/DMS=: branch ✅ | main (4d88efe) ❌ | latest (v1.22.1) ❌main (4d88efe): latest (v1.22.1): same output as main (4d88efe) TestAccept/bundle/refschema/DMS=: branch ✅ | main (4d88efe) ❌ | latest (v1.22.1) ❌main (4d88efe): latest (v1.22.1): 1 more failure details omitted, see the full report in the run. |
Collaborator
Integration test reportCommit: f35452b
Top 6 slowest tests (at least 2 minutes):
|
denik
force-pushed
the
denik/depends-on
branch
2 times, most recently
from
October 20, 2026 13:07
25ceaa6 to
9124e27
Compare
denik
force-pushed
the
denik/depends-on
branch
from
October 20, 2026 13:48
8ea9df4 to
0d4dd13
Compare
denik
added this pull request to stack #7131
October 20, 2026 13:48
denik
marked this pull request as ready for review
October 20, 2026 14:06
pietern
reviewed
Oct 21, 2026
| // DependsOn is a single lifecycle.depends_on entry. | ||
| type DependsOn struct { | ||
| // Key of the resource to depend on: "foo" or "jobs.foo". | ||
| Node string `json:"node"` |
Contributor
There was a problem hiding this comment.
Node is an internal name that isn't part of the configuration anywhere (yet).
I think "resource" fits better because it depends on a resource. If we later add dependencies on other things we can call them by their name too.
denik
force-pushed
the
denik/depends-on
branch
from
October 21, 2026 15:17
9f38d4a to
496252a
Compare
Comment on lines
+10
to
+12
| depends_on: | ||
| - resource: z_job | ||
| - resource: pipelines.z_pipeline |
Member
There was a problem hiding this comment.
might be nice to be able to do this too:
Suggested change
| depends_on: | |
| - resource: z_job | |
| - resource: pipelines.z_pipeline | |
| depends_on: | |
| - ${resources.jobs.y_job} | |
| - resource: z_job | |
| - resource: pipelines.z_pipeline |
Contributor
Author
There was a problem hiding this comment.
why, what would it help with?
I find it confusing because according to our current $ references rules, it literally pastes the definition of y_job into depends_on block.
Member
There was a problem hiding this comment.
it literally pastes the definition of y_job into depends_on
ah, I didn't know we paste verbatim. then nvm.
why, what would it help with?
quickly see from looking at the yml what part of this is static vs dynamic content
Contributor
Author
There was a problem hiding this comment.
not sure what you mean -- it is static, the key of the resource we depend on is a literal.
Member
There was a problem hiding this comment.
poor choice of words, not static vs static but maybe "belongs to the resource no matter what else changes" (its name; other lifecycle fields) vs "value may need to be changed due to changes external to this resource" (table name from another resource; key of another resource).
This is purely visual and of course
lifecycle:
depends_on:
- resources: jobs.foo # -> jobs.barvs
lifecycle:
depends_on:
- ${resources.jobs.foo} # -> ${resources.jobs.bar}will require the exact same refactor if someone renames the key of (& rebinds) the depended-on resource.
I personally would just find the config easier to (re)view with a visual cue that this is a "special" value (a reference to a key, not just a string).
| name: a_job | ||
| lifecycle: | ||
| depends_on: | ||
| - resource: z_job |
Member
There was a problem hiding this comment.
looking at this example I would conclude that z_job only works because it's under the same resources.jobs resource type and z_pipeline needs the pipeline. prefix. But I assume that's not true since we require unique keys. Can you add the other permutations (jobs.z_job and z_pipeline) as well?
Contributor
Author
There was a problem hiding this comment.
I see what you mean, I think it's fine like this because we don't implement resolution of these keys in this PR, it's standard since #7128
Member
There was a problem hiding this comment.
it's standard since #7128
but that PR didn't add acceptance test coverage for that behaviour. I find these example databricks.yml always give a good insight into currently supported behaviour. Would be great to add some (can be a follow-up PR)
janniklasrose
approved these changes
Oct 26, 2026
andrewnester
approved these changes
Oct 26, 2026
| func (*lifecycleDependsOn) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnostics { | ||
| var diags diag.Diagnostics | ||
|
|
||
| pattern := dyn.NewPattern(dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey(), dyn.Key("lifecycle"), dyn.Key("depends_on")) |
Contributor
There was a problem hiding this comment.
You seem to remove dyn usage in separate PR, shall we make this implementation without dyn lib then?
Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
denik
enabled stacked auto-merge
October 26, 2026 10:56
Co-authored-by: Isaac <no-reply@databricks.com>
denik
force-pushed
the
denik/depends-on
branch
from
October 26, 2026 10:58
f35452b to
d8658a0
Compare
denik
disabled stacked auto-merge
October 26, 2026 11:11
Collaborator
Integration test reportCommit: bf9e27b
801 interesting tests: 726 MISS, 75 FAIL
Top 50 slowest tests (at least 2 minutes):
|
Collaborator
Integration test reportCommit: bf9e27b
215 interesting tests: 180 MISS, 27 FAIL, 8 flaky
Top 50 slowest tests (at least 2 minutes):
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Add
lifecycle.depends_on, a list of- resource: <key>entries (fooorjobs.foo) that orders a resource after the listed ones on deploy and before them on destroy. Edges are saved in state like reference-derived ones, including bybundle migrate. A target that is skipped from the plan is ignored.Uses
Resources.ResolveKeyfrom #7128. Used by #7132 to replace the references injected for implicit schema/catalog dependencies.Tests
Acceptance tests for ordering, validation errors, and a config with
depends_onmigrating from Terraform.This pull request and its description were written by Isaac.