Uh oh!
There was an error while loading. Please reload this page.
Normalize clusters in jobs according to Terraform provider logic - #3198
Merged
Conversation
- This helps to prepare the resource for direct deployment, where we don't do any kind of processing. - This makes configuration changes visible to use in bundle validate/summary.
Collaborator
114 failing tests:
|
denik
marked this pull request as ready for review
July 9, 2025 07:52
denik
requested review from
andrewnester, anton-107, pietern and shreyas-goenka
as code ownersJuly 9, 2025 07:52
pietern
reviewed
Jul 9, 2025
Uh oh!
There was an error while loading. Please reload this page.
| c.EnableElasticDisk = false | ||
| c.NodeTypeId = "" | ||
| c.DriverNodeTypeId = "" | ||
| } |
Contributor
There was a problem hiding this comment.
This clears computed fields on the TF side. We don't have that problem here because we exclusively use the configuration as input to our requests. What doesn't work if we don't apply these fixups?
ContributorAuthor
There was a problem hiding this comment.
See the tests that were enabled in this PR.
The field 'node_type_id' cannot be supplied when an instance pool ID is provided."
Other fields are copied from TF for completeness -- If they are output-only it does not hurt to clear them.
Contributor
There was a problem hiding this comment.
They are output only IFF the instance pool is set.
If a user configures both an instance pool and a node type ID, I think we should raise an error.
ContributorAuthor
There was a problem hiding this comment.
Well, that would be a breaking change, so we probably should start with a warning. However, this is out of scope in this PR, here I'm just surfacing what TF already does so that we have parity between deployment backends.
Contributor
There was a problem hiding this comment.
That makes sense. Can you throw in a TODO for good measure so we look at this later?
I prefer removing these modifications if we don't need them once direct is the only deployment mode.
Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>
pietern
approved these changes
Jul 9, 2025
| c.EnableElasticDisk = false | ||
| c.NodeTypeId = "" | ||
| c.DriverNodeTypeId = "" | ||
| } |
Contributor
There was a problem hiding this comment.
That makes sense. Can you throw in a TODO for good measure so we look at this later?
I prefer removing these modifications if we don't need them once direct is the only deployment mode.
Uh oh!
There was an error while loading. Please reload this page.
github-merge-queueBot
pushed a commit
that referenced
this pull request
Jul 10, 2025
…ation (#3226) ## Tests - Move bundle/resources/jobs bundle/resources/jobs/update - Add new test bundle/resources/jobs/instance_pool_and_node_type that tests mutator added in #3198 ## Why We only had slow integration test that checks this behaviour, this is local and explicit and highlights an issue with node_type_id being "" rather than being omitted which trips the backend.
denik added a commit
that referenced
this pull request
May 20, 2026
## Changes Apply changes in resource normalization that are applied by TF and cause invalid configuration to be fixed and accepted by the backend. For example, when InstancePoolId is set, clear NodeTypeId field. ## Why For direct deployment we do all resource transformations in initialize phase rather than before deployment. This makes no effect on the resource - the APIs are called with the same payload because this transformations are idempotent when applied for the second time. However, - this makes it visible to user (via bundle validate -o json) what is the final configuration that is being sent to the backend - makes the same configuration work equally in terraform and direct backend even through direct backend does not transform resources in any way. Same reasoning as in #2767 ## Tests A few tests that were failing on direct backend are now enabled. --------- Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>
denik added a commit
that referenced
this pull request
May 20, 2026
…ation (#3226) ## Tests - Move bundle/resources/jobs bundle/resources/jobs/update - Add new test bundle/resources/jobs/instance_pool_and_node_type that tests mutator added in #3198 ## Why We only had slow integration test that checks this behaviour, this is local and explicit and highlights an issue with node_type_id being "" rather than being omitted which trips the backend.
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.
Changes
Apply changes in resource normalization that are applied by TF and cause invalid configuration to be fixed and accepted by the backend.
For example, when InstancePoolId is set, clear NodeTypeId field.
Why
For direct deployment we do all resource transformations in initialize phase rather than before deployment.
This makes no effect on the resource - the APIs are called with the same payload because this transformations are idempotent when applied for the second time.
However,
Same reasoning as in #2767
Tests
A few tests that were failing on direct backend are now enabled.