Uh oh!
There was an error while loading. Please reload this page.
direct: new 'migrate' impl that looks up values from tfstate without reading resources - #5399
Merged
Conversation
Collaborator
Integration test reportCommit: 730b256
22 interesting tests: 13 SKIP, 7 KNOWN, 2 flaky
Top 4 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
Both callers were reading and parsing the same .tfstate file twice —
once for resource IDs, once for full attributes. Now a single
ParseTFStateFull reads and unmarshals the file once, returning attrs,
IDs, and lineage/serial together.
Also drop the separate `etags map[string]string` parameter from
BuildStateFromTF. The dashboard etag is a regular attribute in the TF
state JSON ("etag" field), so LookupTFField finds it directly without
any special-case plumbing.
Implementation:
- terraform: expose ParseResourcesStateFromBytes so callers can pass
already-read bytes
- migrate: add ParseTFStateFull / parseTFStateAttrsFromBytes
- migrate: remove etags param; look up "etag" via LookupTFField
- tests: etag test now puts the etag in the TF attributes JSON
Co-authored-by: IsaacStill useful as a standalone API; suppress the dead-code checker. Co-authored-by: Isaac
Co-authored-by: Isaac
Return (nil, nil, ...) when the state file doesn't exist, matching the old parseResourcesState behaviour. Empty bundles with no resources don't create a terraform.tfstate file. Co-authored-by: Isaac
Remove the musterr + --noplancheck pattern; the plan check was removed so migration succeeds on the first call. Regenerate output files. Co-authored-by: Isaac
Co-authored-by: Isaac
YAML + TF JSON as input, expected state fields as output. Co-authored-by: Isaac
Co-authored-by: Isaac
Replace the verbose LookupTFField call for etag with a direct JSON read via TFStateAttrs.ETagFor(group, name). Co-authored-by: Isaac
- Introduce rawTFState that captures lineage/serial alongside resources in one unmarshal, eliminating the separate meta struct parse. - parseTFStateAttrsFromBytes and parseTFStateAttrsFromRaw now share the same struct instead of defining an anonymous one. - Move Lineage/Serial after Attrs/IDs in TFState struct. Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
DABsPathToTerraform now correctly handles root-level TF fields for wrapped groups (e.g. postgres) via DABsToTerraformWrapperFields, so the retry-with-unwrapped-path fallback is no longer needed. Co-authored-by: Isaac
- Remove the unused `rest` variable in the alias resolution block (SkipPrefix(1) on a single-segment path always returns nil) - Remove ParseTFStateAttrs which has no callers; the deadcode:allow annotation was papering over the linter Co-authored-by: Isaac
BuildStateFromTF silently skips resources not in TF state (like schema grants in terraform mode), so the old "state entry not found" warning no longer fires. Remove the two spurious warning lines. Co-authored-by: Isaac
The ETag field is no longer needed in the migration path — etags are read directly from TFStateAttrs.ETagFor. Replace ExportedResourcesMap with map[string]string throughout migrate, build_state, and callers. Co-authored-by: Isaac
json.Unmarshal into map[string]any decodes all JSON numbers as float64. Integers beyond 2^53 (~9e15) lose precision: 9007199254740993 becomes 9007199254740992. run_job_task.job_id is a JSON number in TF state and realistic job IDs can exceed this threshold. Fix: use json.NewDecoder.UseNumber() when parsing TF state attributes so numbers are preserved as their original decimal string. Add a unit test case with job_id = 2^53+1 that asserts the raw state JSON contains the exact value, and an invariant config (job_run_job_ref.yml.tmpl) that exercises the run_job_task.job_id cross-reference in cloud tests. Co-authored-by: Isaac
Extend job_run_job_ref so watcher_job references trigger_job's max_concurrent_runs literal of 2^53+1 (9007199254740993). This drives the migration to resolve a large integer from TF state end-to-end; with the json.Number fix the migrated state keeps the exact value, without it the value is truncated to 2^53. The value is out of range for the real backend, so the config is local-only (no_run_job_ref_on_cloud). Note: the unit test (TestBuildStateFromTF/large_integer) is the deterministic regression guard, since the "no drift" plan check cannot detect this — the plan diff also collapses ints to float64. Co-authored-by: Isaac
main's DABsToTerraformRenameMap now maps models.model_id -> registered_model_id (PR #5621), so the manual tfStateFieldAliases map and the alias-fallback branch in LookupTFField are redundant. LookupTFField now translates once and navigates. Co-authored-by: Isaac
Group all type/struct declarations (TFStateAttrs, TFState, rawTFState) at the top of the file, with methods and functions below. Co-authored-by: Isaac
Version was never read (ParseResourcesStateFromBytes validates the state version separately). Order the serial/lineage fields to match their on-disk order in the terraform state file. Co-authored-by: Isaac
ETagFor did its own json.Unmarshal of the attrs blob; the general LookupTFField path already reads any TF-state field. Use it for "etag" too (a resource without an etag returns an error, treated as "no etag"), removing the one-off parser. Co-authored-by: Isaac
Put Serial/Lineage at the top of TFState to match rawTFState's field order (metadata before resource data). Co-authored-by: Isaac
Co-authored-by: Isaac
| return valueA, nil | ||
| } | ||
| // Both succeeded but disagree: prefer longer string and warn. | ||
| if len(valueB) > len(aStr) { |
Contributor
There was a problem hiding this comment.
Why longer value? What is the logic here?
ContributorAuthor
There was a problem hiding this comment.
- The failure to resolve would results in empty string.
- We want some deterministic selection there.
| return "", fmt.Errorf("cannot look up %q: %w", pathString, err) | ||
| } | ||
| result = strings.ReplaceAll(result, "${"+pathString+"}", fmt.Sprintf("%v", value)) |
Contributor
There was a problem hiding this comment.
What if value returned by LookupTFField is non-scalar, like a map[string]any ? Then stringified value might be incorrect because it will be a Go representation not a JSON string if I read this correctly
ContributorAuthor
There was a problem hiding this comment.
This is evaluateTemplate, so the result must be a string. Still, good question whether to use go or json representation, I'm not sure we have that defined anywhere.
| if key, ok := node.StringKey(); ok { | ||
| // Auto-unwrap TF list-blocks: if the current value is a single-element | ||
| // array and the next step wants a map key, descend into element 0. | ||
| if arr, isArr := current.([]any); isArr { |
Contributor
There was a problem hiding this comment.
Should we only auto-unwrap elements with MaxItems:1? Because oter can be genuinely repeated blocks (task, cluster, library) and they are valid multi-element arrays
ContributorAuthor
There was a problem hiding this comment.
I did not know about MaxItems, is it part of tfschema?
We could check that but I'm not sure it adds value -- the goal here is to migrate whatever we have, not validate correctness of user references (and the case you describe can only happen for incorrect references like task.bla).
eng-dev-ecosystem-bot
commented
Jun 24, 2026
Collaborator
Integration test reportCommit: 2de2c79
25 interesting tests: 13 SKIP, 7 KNOWN, 3 FAIL, 2 flaky
Top 3 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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Changes
Why
Not doing network access means it's faster and less likely to fail, which is something we need for auto-migration.
Tests
Existing tests.