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

Add validation for files with a .(resource-name).yml extension #1780

Merged
merged 38 commits into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
aa51b63
Modify SetLocation test utility to take full locations as argument
shreyas-goenka Sep 24, 2024
667fe6b
Add detection and info diagnostic for convention for `.<resource-type…
shreyas-goenka Sep 19, 2024
458dbfb
-
shreyas-goenka Sep 19, 2024
5c351d7
more wip on making it work
shreyas-goenka Sep 24, 2024
e9426be
-
shreyas-goenka Sep 24, 2024
fa6b68c
add unit tests
shreyas-goenka Sep 24, 2024
ccbd199
add info block
shreyas-goenka Sep 24, 2024
3571410
cleanup todos
shreyas-goenka Sep 24, 2024
f3d7f09
fix failing tests
shreyas-goenka Sep 24, 2024
5bf9163
return if diags has error
shreyas-goenka Sep 24, 2024
e31dcc2
fix logic for dedup
shreyas-goenka Sep 24, 2024
894f4aa
fix windows test
shreyas-goenka Sep 24, 2024
fd01824
-
shreyas-goenka Sep 26, 2024
5308ad8
split into summary and detail
shreyas-goenka Sep 26, 2024
d14b1e2
directly pass config value
shreyas-goenka Sep 26, 2024
4734249
convert inline tests to files
shreyas-goenka Sep 26, 2024
d9cf582
fix failing tests
shreyas-goenka Sep 26, 2024
6040e58
complete info to recommendation
shreyas-goenka Sep 26, 2024
4cc0dd1
Merge remote-tracking branch 'origin' into detect-convention
shreyas-goenka Sep 26, 2024
fec73db
address comments
shreyas-goenka Sep 26, 2024
3203858
add comment
shreyas-goenka Sep 26, 2024
28917eb
-
shreyas-goenka Sep 26, 2024
4373e7b
make unit test package separate
shreyas-goenka Sep 26, 2024
65ad2f0
add test for multiple resources
shreyas-goenka Sep 27, 2024
b4bd47f
fail -> not match
shreyas-goenka Sep 27, 2024
dd28a56
address comments
shreyas-goenka Sep 27, 2024
14e73ac
-
shreyas-goenka Sep 27, 2024
e22ba75
remove todo
shreyas-goenka Sep 27, 2024
367048b
account for the oxford comma
shreyas-goenka Sep 27, 2024
dd20a52
terser recommendation message
shreyas-goenka Sep 27, 2024
dd10eed
merge
shreyas-goenka Sep 27, 2024
90f360b
-
shreyas-goenka Sep 27, 2024
a737977
address comments
shreyas-goenka Oct 2, 2024
d064566
Update bundle/config/loader/process_include.go
shreyas-goenka Oct 2, 2024
b288b07
Update bundle/config/loader/process_include.go
shreyas-goenka Oct 2, 2024
b1d7c7b
Update bundle/config/loader/process_include.go
shreyas-goenka Oct 2, 2024
361f590
Update bundle/config/loader/process_include.go
shreyas-goenka Oct 2, 2024
caa8d64
Merge branch 'main' into detect-convention
shreyas-goenka Oct 2, 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
7 changes: 4 additions & 3 deletions bundle/artifacts/expand_globs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/databricks/cli/bundle/config"
"github.com/databricks/cli/bundle/internal/bundletest"
"github.com/databricks/cli/internal/testutil"
"github.com/databricks/cli/libs/dyn"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand All @@ -36,7 +37,7 @@ func TestExpandGlobs_Nominal(t *testing.T) {
},
}

bundletest.SetLocation(b, "artifacts", filepath.Join(tmpDir, "databricks.yml"))
bundletest.SetLocation(b, "artifacts", []dyn.Location{{File: filepath.Join(tmpDir, "databricks.yml")}})

ctx := context.Background()
diags := bundle.Apply(ctx, b, bundle.Seq(
Expand Down Expand Up @@ -77,7 +78,7 @@ func TestExpandGlobs_InvalidPattern(t *testing.T) {
},
}

bundletest.SetLocation(b, "artifacts", filepath.Join(tmpDir, "databricks.yml"))
bundletest.SetLocation(b, "artifacts", []dyn.Location{{File: filepath.Join(tmpDir, "databricks.yml")}})

ctx := context.Background()
diags := bundle.Apply(ctx, b, bundle.Seq(
Expand Down Expand Up @@ -125,7 +126,7 @@ func TestExpandGlobs_NoMatches(t *testing.T) {
},
}

bundletest.SetLocation(b, "artifacts", filepath.Join(tmpDir, "databricks.yml"))
bundletest.SetLocation(b, "artifacts", []dyn.Location{{File: filepath.Join(tmpDir, "databricks.yml")}})

ctx := context.Background()
diags := bundle.Apply(ctx, b, bundle.Seq(
Expand Down
2 changes: 1 addition & 1 deletion bundle/config/loader/entry_point_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func TestEntryPointNoRootPath(t *testing.T) {

func TestEntryPoint(t *testing.T) {
b := &bundle.Bundle{
RootPath: "testdata",
RootPath: "testdata/basic",
}
diags := bundle.Apply(context.Background(), b, loader.EntryPoint())
require.NoError(t, diags.Error())
Expand Down
139 changes: 139 additions & 0 deletions bundle/config/loader/process_include.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,144 @@ package loader
import (
"context"
"fmt"
"sort"
"strings"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config"
"github.com/databricks/cli/libs/diag"
"github.com/databricks/cli/libs/dyn"
"golang.org/x/exp/maps"
)

var resourceTypes = []string{
"job",
"pipeline",
"model",
"experiment",
"model_serving_endpoint",
"registered_model",
"quality_monitor",
"schema",
"cluster",
}
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved

func validateFileFormat(r *config.Root, filePath string) diag.Diagnostics {
for _, typ := range resourceTypes {
for _, ext := range []string{fmt.Sprintf(".%s.yml", typ), fmt.Sprintf(".%s.yaml", typ)} {
Copy link
Contributor

Choose a reason for hiding this comment

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

@fjakobs Can you comment on why we should check both?

Useful context to include here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the plan is to have DABs in the workspace support both .yml and .yaml since asking users to change .yaml -> .yml is a bit arbitrary in a recommendation for DABs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree.

@fjakobs Can you provide a snippet we can include for future readers wrt the expected extensions?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do recommend using yml but it feels a bit arbitrary to actively prohibit this.

It's a SHOULD in https://docs.google.com/document/d/1M4UqmNdpDgKMfuBVSghm_UtaDdYp4r-1SA0SuWsgG_k/edit#heading=h.bs9f118p0tig and not a must.

if strings.HasSuffix(filePath, ext) {
return validateSingleResourceDefined(r, ext, typ)
}
}
}

return nil
}

func validateSingleResourceDefined(r *config.Root, ext, typ string) diag.Diagnostics {
type resource struct {
path dyn.Path
value dyn.Value
typ string
key string
}

resources := []resource{}

// Gather all resources defined in the resources block.
_, err := dyn.MapByPattern(
r.Value(),
dyn.NewPattern(dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey()),
func(p dyn.Path, v dyn.Value) (dyn.Value, error) {
// The key for the resource. Eg: "my_job" for jobs.my_job.
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved
k := p[2].Key()
// The type of the resource. Eg: "job" for jobs.my_job.
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved
typ := strings.TrimSuffix(p[1].Key(), "s")
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved

resources = append(resources, resource{path: p, value: v, typ: typ, key: k})
return v, nil
})
if err != nil {
return diag.FromErr(err)
}

// Gather all resources defined in a target block.
_, err = dyn.MapByPattern(
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved
r.Value(),
dyn.NewPattern(dyn.Key("targets"), dyn.AnyKey(), dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey()),
func(p dyn.Path, v dyn.Value) (dyn.Value, error) {
// The key for the resource. Eg: "my_job" for jobs.my_job.
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved
k := p[4].Key()
// The type of the resource. Eg: "job" for jobs.my_job.
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved
typ := strings.TrimSuffix(p[3].Key(), "s")

resources = append(resources, resource{path: p, value: v, typ: typ, key: k})
return v, nil
})
if err != nil {
return diag.FromErr(err)
}

typeMatch := true
seenKeys := map[string]struct{}{}
for _, rr := range resources {
// case: The resource is not of the correct type.
if rr.typ != typ {
typeMatch = false
break
}

seenKeys[rr.key] = struct{}{}
}

// Format matches. There's less than or equal to one resource defined in the file.
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved
// The resource is also of the correct type.
if typeMatch && len(seenKeys) <= 1 {
return nil
}

msg := strings.Builder{}
msg.WriteString(fmt.Sprintf("We recommend only defining a single %s in a file with the %s extension.\n", typ, ext))

// Dedup the list of resources before adding them the diagnostic message. This
// is needed because we do not dedup earlier when gathering the resources and
// it's valid to define the same resource in both the resources and targets block.
msg.WriteString("The following resources are defined or configured in this file:\n")
setOfLines := map[string]struct{}{}
for _, r := range resources {
setOfLines[fmt.Sprintf(" - %s (%s)\n", r.key, r.typ)] = struct{}{}
}
// Sort the line s to print to make the output deterministic.
listOfLines := maps.Keys(setOfLines)
sort.Strings(listOfLines)
for _, l := range listOfLines {
msg.WriteString(l)
}
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved

locations := []dyn.Location{}
paths := []dyn.Path{}
for _, rr := range resources {
locations = append(locations, rr.value.Locations()...)
paths = append(paths, rr.path)
}
// Sort the locations and paths to make the output deterministic.
sort.Slice(locations, func(i, j int) bool {
return locations[i].String() < locations[j].String()
})
sort.Slice(paths, func(i, j int) bool {
return paths[i].String() < paths[j].String()
})

return diag.Diagnostics{
{
Severity: diag.Info,
Summary: msg.String(),
Locations: locations,
Paths: paths,
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved
},
}
}

type processInclude struct {
fullPath string
relPath string
Expand All @@ -31,6 +163,13 @@ func (m *processInclude) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnos
if diags.HasError() {
return diags
}

// Add any diagnostics associated with the file format.
diags = append(diags, validateFileFormat(this, m.relPath)...)
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved
if diags.HasError() {
return diags
}

err := b.Config.Merge(this)
if err != nil {
diags = diags.Extend(diag.FromErr(err))
Expand Down
Loading
Loading