direct: Resolve ${resources.group.key.id} and order deployment accordingly #3390

denik · 2025-08-11T15:04:52Z

Changes

The "${resources...id}" references are now substituted by direct deployment backend. They are used to build DAG both during planning and apply.

Tests

New test that check jobs update, pipeline recreation and reference going into integer field.

All existing tests that were disabled due to lack of ${resources} support are enabled.

eng-dev-ecosystem-bot · 2025-08-11T15:23:58Z

Run: 17037278580

	Env	✅‌pass	🙈‌skip
✅‌	aws linux	310	479
✅‌	aws windows	311	478
✅‌	aws-ucws linux	423	378
✅‌	aws-ucws windows	424	377
✅‌	azure linux	310	478
✅‌	azure windows	311	477
✅‌	azure-ucws linux	425	375
✅‌	azure-ucws windows	426	374
✅‌	gcp linux	309	480
✅‌	gcp windows	310	479

## Changes Improve error message when converting typed struct to dynamic. ## Why Currently different functions share the message body, so you don't know which field failed to convert. Real example when working on #3390 test acceptance/bundle/deploy/jobs/double-underscore-keys triggers this error condition: Before this change "bundle deploy" fails with: ``` Warn: unable to convert typed configuration to dynamic configuration: unhandled type: string Error: exit error: unhandled type: string ``` After: ``` Warn: unable to convert typed configuration to dynamic configuration: cannot convert int field to dynamic type "string": src=4611686018427387911 ref="4611686018427387911" Error: exit error: cannot convert int field to dynamic type "string": src=4611686018427387911 ref="4611686018427387911" ``` ## Tests Manually, by applying this commit on #3390

andrewnester · 2025-08-13T13:17:02Z

bundle/terranova/graph.go

+		return errors.New("internal error: no db entry")
+	}
+
+	bundle.ApplyFuncContext(ctx, b, func(ctx context.Context, b *bundle.Bundle) {


Why does it need to be inside ApplyFuncContext call and not just Config.Mutate?

ApplyFuncContext ensures that dynamic configuration is copied back to typed one.

Discussed offline. At this moment, we never read typed configuration, so we can postpone this conversion and save some time there. Removed ApplyFunContext - all tests still pass.

andrewnester · 2025-08-13T13:17:51Z

bundle/terranova/graph.go

+			if err != nil {
+				return root, err
+			}
+			root, diags := convert.Normalize(b.Config, root)


Why do we need to normalise here?

Added this comment in the code:

+ // Following resolve_variable_references.go, normalize after variable substitution. + // This fixes the following case: ${resources.jobs.foo.id} is replaced by string "12345" + // This string corresponds to job_id integer field. Normalization converts "12345" to 12345. + // Without normalization there will be an error when converting dynamic value to typed. root, diags := convert.Normalize(b.Config, root)

andrewnester · 2025-08-13T13:19:25Z

libs/dagrun/dagrun.go

 }

-func (g *Graph[N]) AddDirectedEdge(from, to N, label string) error {
-	if from == to {


Don't we want to avoid self loops?

We want to, but we already have diagnostics for loops. I've added new tests with both regular and self loops.

Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>

pietern · 2025-08-18T09:29:28Z

bundle/terranova/apply.go

-				return
-			}
+		if actionType == deployplan.ActionTypeUnset {
+			return


Is this no longer an internal error?

it should be. restored.

I got that wrong. This comment explains what should happen 6c3356a

pietern · 2025-08-18T09:29:44Z

bundle/terranova/apply.go

+
+		// Extract unresolved references from a given node only.
+		// We need to do it here because we're constantly resolving references, so that's where we have latest version.
+		myReferences, err := extractReferences(b.Config.Value(), node)


IIRC this referred to a commented out log line.

pietern · 2025-08-18T09:31:42Z

bundle/terranova/graph.go

+}
+
+func validateRef(root dyn.Value, ref string) (string, string, error) {
+	items := strings.Split(ref, ".")


It now uses a mix of split and dyn.NewPath below. Easier to only use dyn.Path.

denik temporarily deployed to test-trigger-is August 11, 2025 15:04 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 11, 2025 15:12 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 11, 2025 16:00 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 11, 2025 16:07 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 11, 2025 16:19 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 11, 2025 16:30 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 12, 2025 10:44 — with GitHub Actions Inactive

denik mentioned this pull request Aug 12, 2025

Better error message in case of conversion error #3402

Merged

denik force-pushed the denik/plan-dag-wip branch from 1e71956 to 92897e8 Compare August 13, 2025 07:30

denik temporarily deployed to test-trigger-is August 13, 2025 07:30 — with GitHub Actions Inactive

denik force-pushed the denik/plan-dag-wip branch from 92897e8 to 671ac12 Compare August 13, 2025 11:10

denik temporarily deployed to test-trigger-is August 13, 2025 11:10 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 13, 2025 11:14 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 13, 2025 11:16 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 13, 2025 11:19 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 13, 2025 11:40 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 13, 2025 12:19 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 13, 2025 12:24 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 13, 2025 12:28 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is August 13, 2025 12:39 — with GitHub Actions Inactive

denik marked this pull request as ready for review August 13, 2025 12:40

denik requested review from andrewnester, anton-107, pietern and shreyas-goenka as code owners August 13, 2025 12:40

denik temporarily deployed to test-trigger-is August 13, 2025 12:43 — with GitHub Actions Inactive

denik changed the title ~~Resolve $resources.group.key.id and order deployment accordingly~~ direct: Resolve $resources.group.key.id and order deployment accordingly Aug 13, 2025

andrewnester reviewed Aug 13, 2025

View reviewed changes

denik and others added 17 commits August 18, 2025 11:14

clean up

3fa1c1f

add job_id test

29cce55

clean up duplication

ff017e0

clean up; fix "concurrent map access"

fb7adb6

cycles: make messages nicer and add acc tests

3575a16

clean up whitespace and debug output

44e1d19

add FOO_ID and BAR_ID

be05fd3

add a comment

60f4add

clean up tests

8988c85

rm unnecessary ApplyFuncContext

b329b9a

remove dot in error message

2ace963

Add Badness

4d1cd94

clean up commented out log

6d9b115

Update bundle/terranova/graph.go

afda5c2

Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>

clean up + comments

fe234c7

update comment

962e87a

clean up referencedNode

52f4575

denik force-pushed the denik/plan-dag-wip branch from fef7ba0 to 52f4575 Compare August 18, 2025 09:14

denik temporarily deployed to test-trigger-is August 18, 2025 09:14 — with GitHub Actions Inactive

pietern approved these changes Aug 18, 2025

View reviewed changes

add missing err handling

4b25d78

denik temporarily deployed to test-trigger-is August 18, 2025 09:43 — with GitHub Actions Inactive

restore Unset handling

6c3356a

denik temporarily deployed to test-trigger-is August 18, 2025 09:55 — with GitHub Actions Inactive

denik enabled auto-merge August 18, 2025 09:56

denik added this pull request to the merge queue Aug 18, 2025

Merged via the queue into main with commit a76dc13 Aug 18, 2025
13 checks passed

denik deleted the denik/plan-dag-wip branch August 18, 2025 10:33

denik mentioned this pull request Aug 18, 2025

Resolve $resources refs before deployment if set by user #3423

Merged

direct: Resolve ${resources.group.key.id} and order deployment accordingly #3390

direct: Resolve ${resources.group.key.id} and order deployment accordingly #3390

Uh oh!

Conversation

denik commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

eng-dev-ecosystem-bot commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

denik commented Aug 11, 2025 •

edited

Loading

eng-dev-ecosystem-bot commented Aug 11, 2025 •

edited

Loading