From 8053e9c4e48501e1b5476f2cbb329a14a6e0b897 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 28 Nov 2024 13:27:24 +0100 Subject: [PATCH] Fix segfault in bundle summary command (#1937) ## 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 --- bundle/config/resources.go | 6 +++++ bundle/config/resources/clusters.go | 4 +++ bundle/config/resources/dashboard.go | 4 +++ bundle/config/resources/job.go | 4 +++ bundle/config/resources/mlflow_experiment.go | 4 +++ bundle/config/resources/mlflow_model.go | 4 +++ .../resources/model_serving_endpoint.go | 4 +++ bundle/config/resources/pipeline.go | 4 +++ bundle/config/resources/quality_monitor.go | 4 +++ bundle/config/resources/registered_model.go | 4 +++ bundle/config/resources/schema.go | 4 +++ bundle/render/render_text_output_test.go | 4 +++ bundle/resources/completion_test.go | 22 ++++++++++++---- bundle/resources/lookup_test.go | 25 ++++++++++++++----- 14 files changed, 86 insertions(+), 11 deletions(-) diff --git a/bundle/config/resources.go b/bundle/config/resources.go index 0affb6ef0..2886e3571 100644 --- a/bundle/config/resources.go +++ b/bundle/config/resources.go @@ -41,6 +41,9 @@ type ConfigResource interface { // InitializeURL initializes the URL field of the resource. InitializeURL(baseURL url.URL) + + // IsNil returns true if the resource is nil, for example, when it was removed from the bundle. + IsNil() bool } // ResourceGroup represents a group of resources of the same type. @@ -57,6 +60,9 @@ func collectResourceMap[T ConfigResource]( ) ResourceGroup { resources := make(map[string]ConfigResource) for key, resource := range input { + if resource.IsNil() { + continue + } resources[key] = resource } return ResourceGroup{ diff --git a/bundle/config/resources/clusters.go b/bundle/config/resources/clusters.go index eb0247c6e..ba991e865 100644 --- a/bundle/config/resources/clusters.go +++ b/bundle/config/resources/clusters.go @@ -56,3 +56,7 @@ func (s *Cluster) GetName() string { func (s *Cluster) GetURL() string { return s.URL } + +func (s *Cluster) IsNil() bool { + return s.ClusterSpec == nil +} diff --git a/bundle/config/resources/dashboard.go b/bundle/config/resources/dashboard.go index 724b03393..5c58a2f2b 100644 --- a/bundle/config/resources/dashboard.go +++ b/bundle/config/resources/dashboard.go @@ -79,3 +79,7 @@ func (r *Dashboard) GetName() string { func (r *Dashboard) GetURL() string { return r.URL } + +func (r *Dashboard) IsNil() bool { + return r.Dashboard == nil +} diff --git a/bundle/config/resources/job.go b/bundle/config/resources/job.go index 98db1ec5d..0aa41b2e8 100644 --- a/bundle/config/resources/job.go +++ b/bundle/config/resources/job.go @@ -63,3 +63,7 @@ func (j *Job) GetName() string { func (j *Job) GetURL() string { return j.URL } + +func (j *Job) IsNil() bool { + return j.JobSettings == nil +} diff --git a/bundle/config/resources/mlflow_experiment.go b/bundle/config/resources/mlflow_experiment.go index a5871468f..5d179ec0f 100644 --- a/bundle/config/resources/mlflow_experiment.go +++ b/bundle/config/resources/mlflow_experiment.go @@ -58,3 +58,7 @@ func (s *MlflowExperiment) GetName() string { func (s *MlflowExperiment) GetURL() string { return s.URL } + +func (s *MlflowExperiment) IsNil() bool { + return s.Experiment == nil +} diff --git a/bundle/config/resources/mlflow_model.go b/bundle/config/resources/mlflow_model.go index 9ead254d8..72376f45d 100644 --- a/bundle/config/resources/mlflow_model.go +++ b/bundle/config/resources/mlflow_model.go @@ -58,3 +58,7 @@ func (s *MlflowModel) GetName() string { func (s *MlflowModel) GetURL() string { return s.URL } + +func (s *MlflowModel) IsNil() bool { + return s.Model == nil +} diff --git a/bundle/config/resources/model_serving_endpoint.go b/bundle/config/resources/model_serving_endpoint.go index 7f3ae00c8..a3c472b3f 100644 --- a/bundle/config/resources/model_serving_endpoint.go +++ b/bundle/config/resources/model_serving_endpoint.go @@ -66,3 +66,7 @@ func (s *ModelServingEndpoint) GetName() string { func (s *ModelServingEndpoint) GetURL() string { return s.URL } + +func (s *ModelServingEndpoint) IsNil() bool { + return s.CreateServingEndpoint == nil +} diff --git a/bundle/config/resources/pipeline.go b/bundle/config/resources/pipeline.go index b3311d8e2..eaa4c5368 100644 --- a/bundle/config/resources/pipeline.go +++ b/bundle/config/resources/pipeline.go @@ -58,3 +58,7 @@ func (p *Pipeline) GetName() string { func (s *Pipeline) GetURL() string { return s.URL } + +func (s *Pipeline) IsNil() bool { + return s.PipelineSpec == nil +} diff --git a/bundle/config/resources/quality_monitor.go b/bundle/config/resources/quality_monitor.go index 30ec4f918..b1d7e08a5 100644 --- a/bundle/config/resources/quality_monitor.go +++ b/bundle/config/resources/quality_monitor.go @@ -62,3 +62,7 @@ func (s *QualityMonitor) GetName() string { func (s *QualityMonitor) GetURL() string { return s.URL } + +func (s *QualityMonitor) IsNil() bool { + return s.CreateMonitor == nil +} diff --git a/bundle/config/resources/registered_model.go b/bundle/config/resources/registered_model.go index c44526d09..8513a79ae 100644 --- a/bundle/config/resources/registered_model.go +++ b/bundle/config/resources/registered_model.go @@ -68,3 +68,7 @@ func (s *RegisteredModel) GetName() string { func (s *RegisteredModel) GetURL() string { return s.URL } + +func (s *RegisteredModel) IsNil() bool { + return s.CreateRegisteredModelRequest == nil +} diff --git a/bundle/config/resources/schema.go b/bundle/config/resources/schema.go index a9f905cf1..8eadd7e46 100644 --- a/bundle/config/resources/schema.go +++ b/bundle/config/resources/schema.go @@ -56,3 +56,7 @@ func (s *Schema) UnmarshalJSON(b []byte) error { func (s Schema) MarshalJSON() ([]byte, error) { return marshal.Marshal(s) } + +func (s *Schema) IsNil() bool { + return s.CreateSchema == nil +} diff --git a/bundle/render/render_text_output_test.go b/bundle/render/render_text_output_test.go index cd9e7723b..135d79dae 100644 --- a/bundle/render/render_text_output_test.go +++ b/bundle/render/render_text_output_test.go @@ -519,6 +519,10 @@ func TestRenderSummary(t *testing.T) { URL: "https://url2", JobSettings: &jobs.JobSettings{Name: "job2-name"}, }, + "job3": { + ID: "3", + URL: "https://url3", // This emulates deleted job + }, }, Pipelines: map[string]*resources.Pipeline{ "pipeline2": { diff --git a/bundle/resources/completion_test.go b/bundle/resources/completion_test.go index 2f7942aae..80412b6f1 100644 --- a/bundle/resources/completion_test.go +++ b/bundle/resources/completion_test.go @@ -6,6 +6,8 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/databricks/databricks-sdk-go/service/pipelines" "github.com/stretchr/testify/assert" ) @@ -14,11 +16,17 @@ func TestCompletions_SkipDuplicates(t *testing.T) { Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ - "foo": {}, - "bar": {}, + "foo": { + JobSettings: &jobs.JobSettings{}, + }, + "bar": { + JobSettings: &jobs.JobSettings{}, + }, }, Pipelines: map[string]*resources.Pipeline{ - "foo": {}, + "foo": { + PipelineSpec: &pipelines.PipelineSpec{}, + }, }, }, }, @@ -36,10 +44,14 @@ func TestCompletions_Filter(t *testing.T) { Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ - "foo": {}, + "foo": { + JobSettings: &jobs.JobSettings{}, + }, }, Pipelines: map[string]*resources.Pipeline{ - "bar": {}, + "bar": { + PipelineSpec: &pipelines.PipelineSpec{}, + }, }, }, }, diff --git a/bundle/resources/lookup_test.go b/bundle/resources/lookup_test.go index b2eaafd1a..0ea5af7a2 100644 --- a/bundle/resources/lookup_test.go +++ b/bundle/resources/lookup_test.go @@ -7,6 +7,7 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/databricks/databricks-sdk-go/service/pipelines" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -28,8 +29,12 @@ func TestLookup_NotFound(t *testing.T) { Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ - "foo": {}, - "bar": {}, + "foo": { + JobSettings: &jobs.JobSettings{}, + }, + "bar": { + JobSettings: &jobs.JobSettings{}, + }, }, }, }, @@ -45,10 +50,14 @@ func TestLookup_MultipleFound(t *testing.T) { Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ - "foo": {}, + "foo": { + JobSettings: &jobs.JobSettings{}, + }, }, Pipelines: map[string]*resources.Pipeline{ - "foo": {}, + "foo": { + PipelineSpec: &pipelines.PipelineSpec{}, + }, }, }, }, @@ -92,10 +101,14 @@ func TestLookup_NominalWithFilters(t *testing.T) { Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ - "foo": {}, + "foo": { + JobSettings: &jobs.JobSettings{}, + }, }, Pipelines: map[string]*resources.Pipeline{ - "bar": {}, + "bar": { + PipelineSpec: &pipelines.PipelineSpec{}, + }, }, }, },