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.

Sign up for GitHub

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

Jump to bottom

Fixed variable override in target with full variable syntax #1749

Merged
merged 5 commits into from
Sep 4, 2024
Merged

Fixed variable override in target with full variable syntax #1749

merged 5 commits into from
Sep 4, 2024
+86 −21

Conversation

andrewnester
Copy link
Contributor

Changes

This PR makes sure that both of this override syntax for variables work correctly

targets:
  dev:
    variables:
      cluster1:
        spark_version: "14.2.x-scala2.11"
        node_type_id: "Standard_DS3_v2"
        num_workers: 4
        spark_conf:
          spark.speculation: false
          spark.databricks.delta.retentionDurationCheck.enabled: false
      cluster2:
        default:
          spark_version: "14.2.x-scala2.11"
          node_type_id: "Standard_DS3_v2"
          num_workers: 4
          spark_conf:
            spark.speculation: false
            spark.databricks.delta.retentionDurationCheck.enabled: false

Tests

Added regression test

pietern
pietern reviewed Sep 4, 2024
View reviewed changes
bundle/tests/variables/complex_multiple_files/databricks.yml Outdated Show resolved Hide resolved
bundle/config/root.go Outdated Show resolved Hide resolved
bundle/config/root.go Outdated Show resolved Hide resolved
bundle/config/root.go Outdated
return dyn.NewValue(map[string]dyn.Value{
"type": typeV,
"default": variable,
}, variable.Locations()), nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only way to override for complex variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, we support both shorthand and full one. Ambiguity can be if someone defined a complex variable and overrides it with

default: some value goes here

Then we can't really understand if it's shorthand or full syntax

@andrewnester andrewnester requested a review from pietern September 4, 2024 15:10
@andrewnester andrewnester added this pull request to the merge queue Sep 4, 2024
Merged via the queue into main with commit 7203084 Sep 4, 2024
5 checks passed
@andrewnester andrewnester deleted the fix/allow-default-value-override branch September 4, 2024 17:24
andrewnester added a commit that referenced this pull request Sep 5, 2024
@andrewnester
[Release] Release v0.228.0
f78523f 
CLI:
 * Do not error if we cannot prompt for a profile in `auth login` ([#1745](#1745)).

Bundles:
 * Pass along $AZURE_CONFIG_FILE to Terraform process ([#1734](#1734)).
 * Add prompt when a pipeline recreation happens ([#1672](#1672)).
 * Use materialized views in the default-sql template ([#1709](#1709)).
 * Update templates to latest LTS DBR ([#1715](#1715)).
 * Make lock optional in the JSON schema ([#1738](#1738)).
 * Do not suppress normalisation diagnostics for resolving variables ([#1740](#1740)).
 * Include a permissions section in all templates ([#1713](#1713)).
 * Fixed complex variables are not being correctly merged from include files ([#1746](#1746)).
 * Fixed variable override in target with full variable syntax ([#1749](#1749)).

Internal:
 * Consider serverless clusters as compatible for Python wheel tasks ([#1733](#1733)).
 * PythonMutator: explain missing package error ([#1736](#1736)).
 * Add `dyn.Time` to box a timestamp with its original string value ([#1732](#1732)).
 * Fix streaming of stdout, stdin, stderr in cobra test runner ([#1742](#1742)).

Dependency updates:
 * Bump github.com/Masterminds/semver/v3 from 3.2.1 to 3.3.0 ([#1741](#1741)).
@andrewnester andrewnester mentioned this pull request Sep 5, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Sep 5, 2024
@andrewnester @pietern
[Release] Release v0.228.0 (#1752)
f71d9e7 
CLI:
* Do not error if we cannot prompt for a profile in `auth login`
([#1745](#1745)).

Bundles:

As of this release CLI will show a prompt is if there are configuration
changes which will lead to a DLT recreation.
Users can skip the prompt by specifying the `--auto-approve` flag

* Pass along $AZURE_CONFIG_FILE to Terraform process
([#1734](#1734)).
* Add prompt when a pipeline recreation happens
([#1672](#1672)).
* Use materialized views in the default-sql template
([#1709](#1709)).
* Update templates to latest LTS DBR
([#1715](#1715)).
* Make lock optional in the JSON schema
([#1738](#1738)).
* Do not suppress normalisation diagnostics for resolving variables
([#1740](#1740)).
* Include a permissions section in all templates
([#1713](#1713)).
* Fixed complex variables are not being correctly merged from include
files ([#1746](#1746)).
* Fixed variable override in target with full variable syntax
([#1749](#1749)).

Internal:
* Consider serverless clusters as compatible for Python wheel tasks
([#1733](#1733)).
* PythonMutator: explain missing package error
([#1736](#1736)).
* Add `dyn.Time` to box a timestamp with its original string value
([#1732](#1732)).
* Fix streaming of stdout, stdin, stderr in cobra test runner
([#1742](#1742)).

Dependency updates:
* Bump github.com/Masterminds/semver/v3 from 3.2.1 to 3.3.0
([#1741](#1741)).

---------

Co-authored-by: Pieter Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pietern pietern pietern approved these changes

@shreyas-goenka shreyas-goenka Awaiting requested review from shreyas-goenka

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andrewnester @pietern