Skip to content

Commit

Permalink
Fix segfault in bundle summary command (#1937)
Browse files Browse the repository at this point in the history
## 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
  • Loading branch information
andrewnester authored Nov 28, 2024
1 parent 6fc2093 commit 8053e9c
Show file tree
Hide file tree
Showing 14 changed files with 86 additions and 11 deletions.
6 changes: 6 additions & 0 deletions bundle/config/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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{
Expand Down
4 changes: 4 additions & 0 deletions bundle/config/resources/clusters.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
4 changes: 4 additions & 0 deletions bundle/config/resources/dashboard.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
4 changes: 4 additions & 0 deletions bundle/config/resources/job.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
4 changes: 4 additions & 0 deletions bundle/config/resources/mlflow_experiment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
4 changes: 4 additions & 0 deletions bundle/config/resources/mlflow_model.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
4 changes: 4 additions & 0 deletions bundle/config/resources/model_serving_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
4 changes: 4 additions & 0 deletions bundle/config/resources/pipeline.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
4 changes: 4 additions & 0 deletions bundle/config/resources/quality_monitor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
4 changes: 4 additions & 0 deletions bundle/config/resources/registered_model.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
4 changes: 4 additions & 0 deletions bundle/config/resources/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
4 changes: 4 additions & 0 deletions bundle/render/render_text_output_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
22 changes: 17 additions & 5 deletions bundle/resources/completion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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{},
},
},
},
},
Expand All @@ -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{},
},
},
},
},
Expand Down
25 changes: 19 additions & 6 deletions bundle/resources/lookup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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{},
},
},
},
},
Expand All @@ -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{},
},
},
},
},
Expand Down Expand Up @@ -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{},
},
},
},
},
Expand Down

0 comments on commit 8053e9c

Please sign in to comment.