Use deployed name for synced database tables in bundle summary#5639
Use deployed name for synced database tables in bundle summary#5639ilyakuz-db wants to merge 1 commit into
Conversation
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
force-pushed
the
fix/synced-table-summary-ref
branch
from
63ca70e to
64ce952
Compare
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Approval status: pending
|
| Jobs: | ||
| foo: | ||
| Name: ${resources.jobs.变量.id} | ||
| Name: [NUMID] |
There was a problem hiding this comment.
could you add replace_ids.py to script so that instead of NUMID we have concrete replacement.
There was a problem hiding this comment.
The new state-based approach is scoped to synced_database_tables, so it no longer changes job-name resolution — I reverted this unicode_reference change, so the replace_ids.py addition is no longer needed here.
| // from the resolved names. | ||
| mutators = append(mutators, | ||
| mutator.ResolveResourceReferences(), | ||
| mutator.InitializeURLs(), |
There was a problem hiding this comment.
This approach is a bit concerning because it introduces a completely different way to resolve $resources references.
One major difference is that it uses values from config not from the state. This could match but they can also be different if config is changed but not yet deployed.
There could also be other differences simply because it's a completely different approach to resolve these.
Let's explore looking up required fields from the state instead?
There was a problem hiding this comment.
Good call — reworked to read from state instead of resolving against config.
I dropped the config-based resolution entirely (reverted process.go and the resolve_variable_references changes). The fix is now scoped to the resource: SyncedDatabaseTable.GetName() prefers the post-deploy id over the configured Name, and InitializeURL derives the URL from it (skipping unresolved names). The id is the deployed three-part name that StateToBundle already loads from state, so summary reflects the deployed value and there is no new resolution path.
This mirrors PostgresSyncedTable.GetName(), which already handles exactly this case (its doc comment even calls out the ${resources.X.Y.Z} scenario).
Integration test reportCommit: dc900f6
22 interesting tests: 14 SKIP, 7 KNOWN, 1 flaky
Top 24 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
ilyakuz-db
force-pushed
the
fix/synced-table-summary-ref
branch
from
64ce952 to
dc900f6
Compare
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
changed the title
3 participants
Changes
bundle summarynow reports the deployed name and URL for asynced_database_tablesresource whose configured name embeds another resource's name, instead of printing the raw${resources...}reference.For example:
Before (post-deploy summary):
After:
The fix is in the resource itself:
SyncedDatabaseTable.GetName()now prefers the post-deployid(the deployed three-part name, whichStateToBundlealready loads from state) over the configuredName, andInitializeURLderives the URL from it and skips names that aren't a fully-resolved three-part identifier. This mirrorsPostgresSyncedTable.GetName(), which already handles this exact case.Why
A synced table's name embeds another resource's name via a
${resources...}reference. Theresourcesprefix is intentionally not resolved into config during initialize (some such references are only known after deploy), sobundle summary(including the in-workspace Bundle resources view, which callsbundle summary) showed the raw reference and a broken percent-encoded URL.The deployed name is already available —
StateToBundleloads it into the resourceid— so the resource just needs to prefer it for display, the same wayPostgresSyncedTabledoes. Reading from state (rather than re-resolving against config) keeps the summary consistent with what was actually deployed.Tests
acceptance/bundle/resources/synced_database_tables/basic: post-deploy summary now shows the resolved name and URL (and the pre-deploy URL is omitted instead of percent-encoding an unresolved reference).TestSyncedDatabaseTableGetName/TestSyncedDatabaseTableInitializeURLcover preferring the deployed id and skipping the URL for an unresolved name.