Add defaults on jobs that terraform applies #2767
Conversation
|
Are you sure it's applied by Terraform? I think it's server-side default, and the reason why you can see it in Terraform is that it re-fetches resources after creation. |
Pretty sure, here's a test that records /jobs/create request (which is a first request) made by terraform: |
denik
force-pushed
the
denik/jobs-defaults
branch
from
52baa27 to
aae0d3c
Compare
## Why Presets should have more priority and need to see the configuration before common defaults are applied, because this mutator is making decisions based on presence of user-provided fields in the configuration. This does not matter currently where we only have few defaults, but will start to matter once we add more, like in #2767 Previous PR #2777 makes it possible to move this without issues. ## Tests Existing tests for empty resource added in #2771 and #2774
denik
force-pushed
the
denik/jobs-defaults
branch
from
aae0d3c to
028cbe0
Compare
denik
force-pushed
the
denik/jobs-defaults
branch
from
028cbe0 to
0be67e6
Compare
denik
requested review from
pietern,
andrewnester,
shreyas-goenka and
anton-107
as code owners
denik
force-pushed
the
denik/jobs-defaults
branch
from
c31cb35 to
2e2e587
Compare
| ], | ||
| "trigger": { | ||
| - "pause_status": "PAUSED", | ||
| + "pause_status": "UNPAUSED", |
There was a problem hiding this comment.
I suspect this is the diff between dev and prod, indicating that the default was not sent for prod.
Can you confirm?
There was a problem hiding this comment.
This is indeed a diff between dev and prod output for "validate -o json" and the pause_status=UNPAUSED is now sent for prod but it was not before:
-
(NEW) pause_status: UNPAUSED is present in prod configuration. It comes from defaults added in this PR. It matches what terraform would add anyway. (We'll be able to verify that better once RecordRequests for integration tests by @shreyas-goenka is landed Add support for recording requests in integration acceptance tests #2720)
-
(UNCHANGED) pause_status: PAUSED is present in dev configuration. It comes from ApplyTargetMode.
|
|
||
| // https://github.com/databricks/terraform-provider-databricks/blob/v1.75.0/clusters/resource_cluster.go | ||
| // This triggers SingleNodeCluster() cluster validator. It needs to be run before applying defaults. | ||
| //{"resources.jobs.*.job_clusters[*].new_cluster.num_workers", 0}, |
There was a problem hiding this comment.
I'm curious to know why this is a TF default to begin with. Can you check in with the TF team to figure this out?
denik
force-pushed
the
denik/jobs-defaults
branch
from
a91e746 to
527f6d2
Compare
## Changes Set defaults for pipelines: edition and channel, same as added by terraform. ## Why See #2767 ## Tests Existing tests.
Changes
Initialize jobs resources with the defaults that terraform applies (except for num_workers).
Why
This makes those defaults visible for users (bundle validate -o json) and PyDABs.
This makes terraform-removal branch simpler as there is no hidden modification of resources that terraform does on deploy.
num_workers is skipped because it has a conflict with SingleNodeCluster validator. We should find a better place for that (and other validators), somewhere before applying defaults.