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

Fix segfault in bundle summary command #1937

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Conversation

andrewnester
Copy link
Contributor

@andrewnester andrewnester commented Nov 27, 2024

Changes

This PR introduces use of new isNil method. It allows to ensure we filter out all improperly defined resources in bundle summary command. This includes deleted resources or resources with incorrect configuration such as only defining key of the resource and nothing else.

Fixes #1919, #1913

Tests

Added regression unit test case

@pietern
Copy link
Contributor

pietern commented Nov 27, 2024

This happens because of the terraform.Load we run for the summary command?

@andrewnester
Copy link
Contributor Author

@pietern yes, strictly speaking it's correct because such jobs are marked as deleted, but it is also useful if someone defines just a job key and no other job settings which makes job to be nil in the config

Copy link

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 1937
  • Commit SHA: 69f5d3466b44b926c5291e92b2a6f7b4c7a46934

Checks will be approved automatically on success.

@eng-dev-ecosystem-bot
Copy link
Collaborator

Test Details: go/deco-tests/12051882555

"job3": {
ID: "3",
URL: "https://url3", // This emulates deleted job
},
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, this PR would allow such config to be loaded and deployed but will skip over this particular job.

Why don't we reject such jobs at validation stage instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly. This is a "bundle summary" command which essentially shows the current state of your bundle configuration. #1123
It is used in VS Code for rendering the state of resources in the side panel as well as directly by customers to see the state of the bundle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For validation stage we have a mutator which does check that all resources are defined correctly https://github.com/databricks/cli/blob/main/bundle/config/validate/all_resources_have_values.go#L14

@andrewnester andrewnester requested a review from denik November 28, 2024 11:10
@andrewnester andrewnester added this pull request to the merge queue Nov 28, 2024
Merged via the queue into main with commit 8053e9c Nov 28, 2024
10 checks passed
@andrewnester andrewnester deleted the fix/summary-segfault branch November 28, 2024 12:35
pietern added a commit that referenced this pull request Dec 5, 2024
**New features for Databricks Asset Bundles:**

This release adds support for managing Unity Catalog volumes as part of your bundle configuration.

Bundles:
 * Add DABs support for Unity Catalog volumes ([#1762](#1762)).
 * Support lookup by name of notification destinations ([#1922](#1922)).
 * Extend "notebook not found" error to warn about missing extension ([#1920](#1920)).
 * Skip sync warning if no sync paths are defined ([#1926](#1926)).
 * Add validation for single node clusters ([#1909](#1909)).
 * Fix segfault in bundle summary command ([#1937](#1937)).
 * Add the `bundle_uuid` helper function for templates ([#1947](#1947)).
 * Add default value for `volume_type` for DABs ([#1952](#1952)).
 * Properly read Git metadata when running inside workspace ([#1945](#1945)).
 * Upgrade TF provider to 1.59.0 ([#1960](#1960)).

Internal:
 * Breakout variable lookup into separate files and tests ([#1921](#1921)).
 * Add golangci-lint v1.62.2 ([#1953](#1953)).

Dependency updates:
 * Bump golang.org/x/term from 0.25.0 to 0.26.0 ([#1907](#1907)).
 * Bump github.com/Masterminds/semver/v3 from 3.3.0 to 3.3.1 ([#1930](#1930)).
 * Bump github.com/stretchr/testify from 1.9.0 to 1.10.0 ([#1932](#1932)).
 * Bump github.com/databricks/databricks-sdk-go from 0.51.0 to 0.52.0 ([#1931](#1931)).
github-merge-queue bot pushed a commit that referenced this pull request Dec 5, 2024
**New features for Databricks Asset Bundles:**

This release adds support for managing Unity Catalog volumes as part of
your bundle configuration.

Bundles:
* Add DABs support for Unity Catalog volumes
([#1762](#1762)).
* Support lookup by name of notification destinations
([#1922](#1922)).
* Extend "notebook not found" error to warn about missing extension
([#1920](#1920)).
* Skip sync warning if no sync paths are defined
([#1926](#1926)).
* Add validation for single node clusters
([#1909](#1909)).
* Fix segfault in bundle summary command
([#1937](#1937)).
* Add the `bundle_uuid` helper function for templates
([#1947](#1947)).
* Add default value for `volume_type` for DABs
([#1952](#1952)).
* Properly read Git metadata when running inside workspace
([#1945](#1945)).
* Upgrade TF provider to 1.59.0
([#1960](#1960)).

Internal:
* Breakout variable lookup into separate files and tests
([#1921](#1921)).
* Add golangci-lint v1.62.2
([#1953](#1953)).

Dependency updates:
* Bump golang.org/x/term from 0.25.0 to 0.26.0
([#1907](#1907)).
* Bump github.com/Masterminds/semver/v3 from 3.3.0 to 3.3.1
([#1930](#1930)).
* Bump github.com/stretchr/testify from 1.9.0 to 1.10.0
([#1932](#1932)).
* Bump github.com/databricks/databricks-sdk-go from 0.51.0 to 0.52.0
([#1931](#1931)).
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.

Bundle Summary SIGSEGV
4 participants