Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use dyn.Value as input to generating Terraform JSON #1218

Merged
merged 154 commits into from
Feb 16, 2024
Merged

Conversation

pietern
Copy link
Contributor

@pietern pietern commented Feb 16, 2024

Changes

This builds on #1098 and uses the dyn.Value representation of the bundle configuration to generate the Terraform JSON definition of resources in the bundle.

The existing code (in BundleToTerraform) was not great and in an effort to slightly improve this, I added a package tfdyn that includes dedicated files for each resource type. Every resource type has its own conversion type that takes the dyn.Value of the bundle-side resource and converts it into Terraform resources (e.g. a job and optionally its permissions).

Because we now use a dyn.Value as input, we can represent and emit zero-values that have so far been omitted. For example, setting num_workers: 0 in your bundle configuration now propagates all the way to the Terraform JSON definition.

Tests

  • Unit tests for every converter. I reused the test inputs from convert_test.go.
  • Equivalence tests in every existing test case checks that the resulting JSON is identical.
  • I manually compared the TF JSON file generated by the CLI from the main branch and from this PR on all of our bundles and bundle examples (internal and external) and found the output doesn't change (with the exception of the odd zero-value being included by the version in this PR).

This change adds:
* A `config.Walk` function to walk a configuration tree
* A `config.Path` type to represent a value's path inside a tree
* Functions to create a `config.Path` from a string, or convert one to a string
Base automatically changed from bundle-use-dyn to main February 16, 2024 19:49
@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2024

Codecov Report

Attention: 74 lines in your changes are missing coverage. Please review.

Comparison is base (87dd46a) 52.13% compared to head (faab089) 52.53%.

Files Patch % Lines
bundle/deploy/terraform/convert.go 57.14% 13 Missing and 5 partials ⚠️
bundle/deploy/terraform/tfdyn/convert_job.go 72.72% 10 Missing and 5 partials ⚠️
bundle/deploy/terraform/write.go 0.00% 8 Missing ⚠️
bundle/deploy/terraform/tfdyn/convert_pipeline.go 80.00% 4 Missing and 2 partials ⚠️
bundle/deploy/terraform/tfdyn/rename_keys.go 72.72% 4 Missing and 2 partials ⚠️
libs/dyn/path.go 0.00% 4 Missing ⚠️
bundle/deploy/terraform/tfdyn/convert.go 40.00% 3 Missing ⚠️
...undle/deploy/terraform/tfdyn/convert_experiment.go 86.36% 2 Missing and 1 partial ⚠️
bundle/deploy/terraform/tfdyn/convert_model.go 86.36% 2 Missing and 1 partial ⚠️
.../terraform/tfdyn/convert_model_serving_endpoint.go 86.36% 2 Missing and 1 partial ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1218      +/-   ##
==========================================
+ Coverage   52.13%   52.53%   +0.39%     
==========================================
  Files         298      308      +10     
  Lines       16865    17160     +295     
==========================================
+ Hits         8793     9015     +222     
- Misses       7427     7481      +54     
- Partials      645      664      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pietern
Copy link
Contributor Author

pietern commented Feb 16, 2024

Triggered integration tests again and everything passes.

@pietern pietern added this pull request to the merge queue Feb 16, 2024
Merged via the queue into main with commit f70ec35 Feb 16, 2024
4 checks passed
@pietern pietern deleted the bundle-tf-zero-workers branch February 16, 2024 21:02
andrewnester added a commit that referenced this pull request Feb 20, 2024
CLI:
 * Add support for UC Volumes to the `databricks fs` commands ([#1209](#1209)).

Bundles:
 * Use dynamic configuration model in bundles ([#1098](#1098)).
 * Allow use of variables references in primitive non-string fields ([#1219](#1219)).
 * Add an experimental default-sql template ([#1051](#1051)).
 * Add an experimental dbt-sql template ([#1059](#1059)).

Internal:
 * Add fork-user to winget release workflow ([#1214](#1214)).
 * Use `any` as type for data sources and resources in `tf/schema` ([#1216](#1216)).
 * Avoid infinite recursion when normalizing a recursive type ([#1213](#1213)).
 * Fix issue where interpolating a new ref would rewrite unrelated fields ([#1217](#1217)).
 * Use `dyn.Value` as input to generating Terraform JSON ([#1218](#1218)).

API Changes:
 * Changed `databricks lakehouse-monitors update` command with new required argument order.
 * Added `databricks online-tables` command group.

OpenAPI commit cdd76a98a4fca7008572b3a94427566dd286c63b (2024-02-19)
Dependency updates:
 * Bump Terraform provider to v1.36.2 ([#1215](#1215)).
 * Bump github.com/databricks/databricks-sdk-go from 0.32.0 to 0.33.0 ([#1222](#1222)).
@andrewnester andrewnester mentioned this pull request Feb 20, 2024
github-merge-queue bot pushed a commit that referenced this pull request Feb 20, 2024
CLI:
* Add support for UC Volumes to the `databricks fs` commands
([#1209](#1209)).

Bundles:
* Use dynamic configuration model in bundles
([#1098](#1098)).
* Allow use of variables references in primitive non-string fields
([#1219](#1219)).
* Add an experimental default-sql template
([#1051](#1051)).
* Add an experimental dbt-sql template
([#1059](#1059)).

Internal:
* Add fork-user to winget release workflow
([#1214](#1214)).
* Use `any` as type for data sources and resources in `tf/schema`
([#1216](#1216)).
* Avoid infinite recursion when normalizing a recursive type
([#1213](#1213)).
* Fix issue where interpolating a new ref would rewrite unrelated fields
([#1217](#1217)).
* Use `dyn.Value` as input to generating Terraform JSON
([#1218](#1218)).

API Changes:
* Changed `databricks lakehouse-monitors update` command with new
required argument order.
 * Added `databricks online-tables` command group.

OpenAPI commit cdd76a98a4fca7008572b3a94427566dd286c63b (2024-02-19)
Dependency updates:
* Bump Terraform provider to v1.36.2
([#1215](#1215)).
* Bump github.com/databricks/databricks-sdk-go from 0.32.0 to 0.33.0
([#1222](#1222)).
github-merge-queue bot pushed a commit that referenced this pull request Oct 18, 2024
## Changes

In #1218, the `BundleToTerraform` function was replaced by a version
based on the dynamic configuration tree. Since then, it has only been
used in tests to confirm that the output of the old function was equal
to the output of the new function. We no longer need this and can
exclusively rely on the version based on the dynamic configuration tree.

## Tests

Tests pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants