Uh oh!
There was an error while loading. Please reload this page.
internal: update GetResourceConfig to use dyn/convert rather than JSON roundtrip - #3646
Merged
Conversation
deniktemporarily deployed
to
test-trigger-is
September 23, 2025 12:53 — with 
GitHub Actions
Inactive
denik
marked this pull request as ready for review
September 23, 2025 12:53
denik
requested review from
andrewnester, anton-107, pietern and shreyas-goenka
as code ownersSeptember 23, 2025 12:53
Collaborator
|
denikforce-pushed
the
denik/update-get-resource-config
branch
from
September 23, 2025 19:31
99d95fc to
143be1bComparedeniktemporarily deployed
to
test-trigger-is
September 23, 2025 19:31 — with GitHub Actions
Inactive
deniktemporarily deployed
to
test-trigger-is
September 23, 2025 19:34 — with GitHub Actions
Inactive
denik
marked this pull request as draft
September 23, 2025 19:43
denikforce-pushed
the
denik/dyn-is-zero
branch
from
September 24, 2025 09:36
4897b39 to
95c3fe1Comparegithub-merge-queueBot
pushed a commit
that referenced
this pull request
Sep 24, 2025
## Changes - Fix IsZero() method for types like int64. This fixes ToConvert for NumWorkers field in ClusterSpec. - Do not panic in dyn.V() if other int types are encountered. - If panic does happen, include type name. ## Why ToConvert() depends on it and is broken. This unblocks #3650#3646 ## Tests New unit tests.
denikforce-pushed
the
denik/update-get-resource-config
branch
from
September 24, 2025 14:43
1c351e3 to
9bac490Comparedeniktemporarily deployed
to
test-trigger-is
September 24, 2025 14:43 — with GitHub Actions
Inactive
denik
marked this pull request as ready for review
September 24, 2025 14:45
denikforce-pushed
the
denik/update-get-resource-config
branch
from
September 24, 2025 14:52
9bac490 to
f79fdacComparedeniktemporarily deployed
to
test-trigger-is
September 24, 2025 14:52 — with GitHub Actions
Inactive
denikforce-pushed
the
denik/update-get-resource-config
branch
from
September 25, 2025 12:32
f79fdac to
57a975bComparedeniktemporarily deployed
to
test-trigger-is
September 25, 2025 12:33 — with GitHub Actions
Inactive
denikforce-pushed
the
denik/update-get-resource-config
branch
from
September 25, 2025 15:07
57a975b to
55ce74fComparedeniktemporarily deployed
to
test-trigger-is
September 25, 2025 15:07 — with GitHub Actions
Inactive
denikforce-pushed
the
denik/update-get-resource-config
branch
from
September 25, 2025 18:43
55ce74f to
c9d3f45Comparedeniktemporarily deployed
to
test-trigger-is
September 25, 2025 18:43 — with GitHub Actions
Inactive
pietern
approved these changes
Sep 26, 2025
| if err := json.Unmarshal(bytes, typedConfigPtr.Interface()); err != nil { | ||
| err = convert.ToTyped(typedConfigPtr.Interface(), v) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
IIUC this is the core of the change, on top of #3650. Correct?
ContributorAuthor
There was a problem hiding this comment.
Right! I updated the base to make it clear.
denikforce-pushed
the
denik/update-get-resource-config
branch
from
September 26, 2025 10:33
c9d3f45 to
ce4795bComparedeniktemporarily deployed
to
test-trigger-is
September 26, 2025 10:33 — with GitHub Actions
Inactive
denikforce-pushed
the
denik/update-get-resource-config
branch
from
September 26, 2025 10:34
ce4795b to
356e7acComparedeniktemporarily deployed
to
test-trigger-is
September 26, 2025 10:34 — with GitHub Actions
Inactive
denikforce-pushed
the
denik/update-get-resource-config
branch
from
September 26, 2025 10:38
356e7ac to
b23495bComparedeniktemporarily deployed
to
test-trigger-is
September 26, 2025 10:38 — with GitHub Actions
Inactive
github-merge-queueBot
pushed a commit
that referenced
this pull request
Sep 26, 2025
## Changes Fix ToTyped / FromTyped wrt ForceSendFields when there are (multiple) embedded structs. ## Why Incorrect conversion between typed & dynamic value can result in subtle bugs. In direct engine it is even more important, as we rely on typed structs more here. Enables #3646 ## Tests New unit tests.
add test forcesendfield fix another test with multiple updatge and fix another test add 3 tests, need clean up add new test fix clean up clean up lint fix add update_single_node test
denikforce-pushed
the
denik/update-get-resource-config
branch
from
September 26, 2025 11:51
b23495b to
79b009dComparedeniktemporarily deployed
to
test-trigger-is
September 26, 2025 11:51 — with GitHub Actions
Inactive
denik
enabled auto-merge
September 26, 2025 11:51
github-merge-queueBot
removed this pull request from the merge queue due to failed status checks
Sep 26, 2025
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
github-merge-queueBot
pushed a commit
that referenced
this pull request
Sep 30, 2025
## Changes The JSON plan (output of “bundle debug plan”) is now input to “bundle deploy” internally. This means it contains full information about resources about to be deployed. The JSON plan stores configs as well as all unresolved fields as map (path -> reference). There is new module, structvar, that supports resolving these references. It does it quickly and without copying the full config for every resolution, like the previous implementation did. The JSON plan also stores per-field action. This is purely informational, it gives an idea why a certain action for the resource was chosen. During plan and apply, variable resolution is now done in the node that contains the reference. Previously, it was updated by the node that was referenced. This results in better error messages when resolution does not work and it does not fail resources that themselves have no issues (but have incorrect references pointing to them). References can now refer to missing field and it works like in terraform (missing_string_field test). There is slight simplification in how $resources are resolved. All local resolutions (from config) are now done in plan phase. All remote resolutions (from remote state) are now done in apply phase. Previously we could resolve some remote references in plan stage if action was “skip”. This might result in slower deploys, so this is a potential perf regression, to be revisited in the future. Depends on #3650 & #3646 Other changes: - dagrun.Graph now exports its fields. dagrun.OutgoingLabels() is removed. - new function structaccess.Set() to set any field in a struct by path (used by structvar). - dresources.ClassifyByTriggers([]Change) is replaced by dresources.ClassifyByTriggers(Change), aggregation logic now lives in bundle_plan.go ## Why Having the plan as input to deploy provides a view into deployment that we did not have before, allowing to see exactly what is going to be deployed and why. This is useful for: - debugging (why is my pipeline being recreated?) - see diff between deployed infra and config - deploying exactly what was in the diff and nothing else: "bundle deploy -readplan plan.json" can be implemented easily after this change.
denik added a commit
that referenced
this pull request
May 20, 2026
## Changes - Fix IsZero() method for types like int64. This fixes ToConvert for NumWorkers field in ClusterSpec. - Do not panic in dyn.V() if other int types are encountered. - If panic does happen, include type name. ## Why ToConvert() depends on it and is broken. This unblocks #3650#3646 ## Tests New unit tests.
denik added a commit
that referenced
this pull request
May 20, 2026
## Changes Fix ToTyped / FromTyped wrt ForceSendFields when there are (multiple) embedded structs. ## Why Incorrect conversion between typed & dynamic value can result in subtle bugs. In direct engine it is even more important, as we rely on typed structs more here. Enables #3646 ## Tests New unit tests.
denik added a commit
that referenced
this pull request
May 20, 2026
…N roundtrip (#3646) ## Why For direct, we fetch configs as struct instances via GetResourceConfig, which is using json roundtrip between dyn value and struct. That, however, does not work well when there references in the config, see #3645 for details. This switches to converting dyn.Value with dyn/convert ToTyped function. The function was not used previously because it did not handle ForceSendFields correctly for embedded structs which caused JSON marshaller in Go SDK to return error. This was fixed in #3650 and #3649 ## Tests Existing tests.
denik added a commit
that referenced
this pull request
May 20, 2026
## Changes The JSON plan (output of “bundle debug plan”) is now input to “bundle deploy” internally. This means it contains full information about resources about to be deployed. The JSON plan stores configs as well as all unresolved fields as map (path -> reference). There is new module, structvar, that supports resolving these references. It does it quickly and without copying the full config for every resolution, like the previous implementation did. The JSON plan also stores per-field action. This is purely informational, it gives an idea why a certain action for the resource was chosen. During plan and apply, variable resolution is now done in the node that contains the reference. Previously, it was updated by the node that was referenced. This results in better error messages when resolution does not work and it does not fail resources that themselves have no issues (but have incorrect references pointing to them). References can now refer to missing field and it works like in terraform (missing_string_field test). There is slight simplification in how $resources are resolved. All local resolutions (from config) are now done in plan phase. All remote resolutions (from remote state) are now done in apply phase. Previously we could resolve some remote references in plan stage if action was “skip”. This might result in slower deploys, so this is a potential perf regression, to be revisited in the future. Depends on #3650 & #3646 Other changes: - dagrun.Graph now exports its fields. dagrun.OutgoingLabels() is removed. - new function structaccess.Set() to set any field in a struct by path (used by structvar). - dresources.ClassifyByTriggers([]Change) is replaced by dresources.ClassifyByTriggers(Change), aggregation logic now lives in bundle_plan.go ## Why Having the plan as input to deploy provides a view into deployment that we did not have before, allowing to see exactly what is going to be deployed and why. This is useful for: - debugging (why is my pipeline being recreated?) - see diff between deployed infra and config - deploying exactly what was in the diff and nothing else: "bundle deploy -readplan plan.json" can be implemented easily after this change.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Why
For direct, we fetch configs as struct instances via GetResourceConfig, which is using json roundtrip between dyn value and struct. That, however, does not work well when there references in the config, see #3645 for details.
This switches to converting dyn.Value with dyn/convert ToTyped function. The function was not used previously because it did not handle ForceSendFields correctly for embedded structs which caused JSON marshaller in Go SDK to return error. This was fixed in #3650 and #3649
Tests
Existing tests.