Make experimental YAML-sync state upload best-effort#5524
Merged
Conversation
The UploadStateForYamlSync mutator runs after a successful deploy when DATABRICKS_BUNDLE_ENABLE_EXPERIMENTAL_YAML_SYNC is set. Its state conversion reuses direct-engine code (SecretScopeFixups, CalculatePlan, Apply) that reports failures via logdiag on the shared deploy context, flipping the whole deploy to exit 1. A schema with an empty grants list triggers this: terraform emits no grants resource for an empty list, so the migration plan has a node with no state entry. Collect the conversion's diagnostics in an isolated logdiag scope and downgrade them to warnings, consistent with the mutator's existing best-effort error handling. Skip uploading the snapshot when conversion failed so a partial snapshot is never published. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
Integration test reportCommit: b0517cf
26 interesting tests: 15 SKIP, 7 KNOWN, 4 flaky
Top 23 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
Integration test reportCommit: 676e000
486 interesting tests: 398 MISS, 51 FAIL, 21 RECOVERED, 8 KNOWN, 4 PANIC, 2 SKIP, 2 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
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
Found during a full-repo review of the CLI. The experimental YAML-sync state upload (gated behind
DATABRICKS_BUNDLE_ENABLE_EXPERIMENTAL_YAML_SYNC) runs after a deploy has already succeeded and is meant to be best-effort: its own errors are logged as warnings and the deploy continues. But the state conversion it performs reuses direct-engine code (SecretScopeFixups,CalculatePlan,DeploymentBundle.Apply) that reports failures throughlogdiagon the shared deploy context. Any such failure flips a successful deploy to exit 1.A concrete trigger: a schema with an empty
grants: []block. Terraform emits no grants resource for an empty list, so the converted state has no entry for the grants plan node, and the deploy fails withError: state entry not found for "resources.schemas.schema1.grants"even though all resources deployed fine.Changes
Before, a conversion failure inside the YAML-sync upload failed the whole deploy with exit 1; now it is logged as warnings and the deploy completes normally.
libs/logdiag: addedIsolatedContext, which returns a child context with a fresh diagnostics state shadowing the parent's.InitContextnow reuses it.bundle/statemgmt/upload_state_for_yaml_sync.go: the mutator runs its work in an isolated, collecting logdiag scope and downgrades any collected diagnostics tolog.Warnf, consistent with its existing best-effort error handling. SinceDeploymentBundle.Applyreports failures only through logdiag,convertStatenow checks the isolated scope after it and returns an error instead of uploading a snapshot that is missing entries for the failed resources.Test plan
acceptance/bundle/deploy/yaml-sync-empty-grants: deploys a schema withgrants: []and the env var set; before the fix it exited 1 withError: state entry not found, now it completes with warnings and exit 0.TestIsolatedContextverifying diagnostics logged through the isolated scope do not leak to the parent context.go test ./libs/logdiag/ ./bundle/statemgmt/ ./bundle/direct/passes../task fmt-q,./task lint-q, and./task checkspass.This pull request and its description were written by Isaac.