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

Fix segfault in bundle summary command #1937

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Fix segfault in bundle summary command #1937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0db9afc
Fix segfault in bundle summary command
andrewnester Nov 27, 2024
69f5d34
fixed tests
andrewnester Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions bundle/config/resources.go
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
},
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

},
Pipelines: map[string]*resources.Pipeline{
"pipeline2": {
Expand Down
22 changes: 17 additions & 5 deletions bundle/resources/completion_test.go
View file
Open in desktop
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
View file
Open in desktop
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
Loading