Skip to content

Commit 0337224

Browse files
feat(wanda): Add params field for env var validation and discovery
Add `params` field to wanda spec for declaring allowed values of environment variables used in templated fields (name, froms). This enables: - Strict validation: reject builds where env var values don't match declared params - Dependency discovery: specs with params are indexed under all their expanded names (e.g., base$PY with params [3.10, 3.11] is indexed as both base3.10 and base3.11), enabling dependency resolution without requiring all env vars to be set Validation runs before variable expansion in buildDepGraph, so errors reference the original $VARNAME. Topic: wanda-params Relative: wanda-build-deps Signed-off-by: andrew <andrew@anyscale.com>
1 parent 05c0ded commit 0337224

File tree

4 files changed

+611
-38
lines changed

4 files changed

+611
-38
lines changed

wanda/deps.go

Lines changed: 49 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,14 @@ func (g *depGraph) loadSpec(specPath string, isRoot bool) error {
6969
if err != nil {
7070
return fmt.Errorf("parse %s: %w", specPath, err)
7171
}
72+
73+
if err := spec.ValidateParams(g.lookup); err != nil {
74+
return fmt.Errorf("%s: %w", specPath, err)
75+
}
76+
7277
spec = spec.expandVar(g.lookup)
7378

74-
if err := checkUnexpandedVars(spec, specPath); err != nil {
79+
if err := checkUnexpandedVars(spec, specPath, spec.Params); err != nil {
7580
return err
7681
}
7782

@@ -237,8 +242,9 @@ func (g *depGraph) validateDeps() error {
237242
}
238243

239244
// checkUnexpandedVars checks if a spec has any unexpanded environment variables
240-
// and returns a helpful error message if so.
241-
func checkUnexpandedVars(spec *Spec, specPath string) error {
245+
// and returns a helpful error message. If params are declared for a missing var,
246+
// the valid values are included in the error message.
247+
func checkUnexpandedVars(spec *Spec, specPath string, params map[string][]string) error {
242248
var missing []string
243249

244250
if vars := findUnexpandedVars(spec.Name); len(vars) > 0 {
@@ -263,41 +269,33 @@ func checkUnexpandedVars(spec *Spec, specPath string) error {
263269
}
264270
}
265271

266-
if len(unique) == 1 {
267-
return fmt.Errorf("%s: environment variable %s is not set", specPath, unique[0])
272+
// Build error message with param hints where available
273+
var parts []string
274+
for _, v := range unique {
275+
varName := strings.TrimPrefix(v, "$")
276+
if allowed, ok := params[varName]; ok && len(allowed) > 0 {
277+
parts = append(parts, fmt.Sprintf("%s (valid values: %s)", v, strings.Join(allowed, ", ")))
278+
} else {
279+
parts = append(parts, v)
280+
}
268281
}
269-
return fmt.Errorf("%s: environment variables not set: %s", specPath, strings.Join(unique, ", "))
282+
283+
if len(parts) == 1 {
284+
return fmt.Errorf("%s: environment variable %s is not set", specPath, parts[0])
285+
}
286+
return fmt.Errorf("%s: environment variables not set: %s", specPath, strings.Join(parts, "; "))
270287
}
271288

272289
// findUnexpandedVars finds $VAR patterns in a string that were not expanded.
290+
// Returns variable references in $VAR format.
273291
func findUnexpandedVars(s string) []string {
274-
var vars []string
275-
for i := 0; i < len(s); i++ {
276-
if s[i] == '$' && i+1 < len(s) {
277-
// Skip $$
278-
if s[i+1] == '$' {
279-
i++
280-
continue
281-
}
282-
// Find the variable name
283-
j := i + 1
284-
for j < len(s) {
285-
c := s[j]
286-
if c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' || c == '_' {
287-
j++
288-
continue
289-
}
290-
if c >= '0' && c <= '9' && j > i+1 {
291-
j++
292-
continue
293-
}
294-
break
295-
}
296-
if j > i+1 {
297-
vars = append(vars, s[i:j])
298-
}
299-
i = j - 1
300-
}
292+
names := extractVarNames(s)
293+
if len(names) == 0 {
294+
return nil
295+
}
296+
vars := make([]string, len(names))
297+
for i, name := range names {
298+
vars[i] = "$" + name
301299
}
302300
return vars
303301
}
@@ -331,7 +329,8 @@ type discovered struct {
331329
}
332330

333331
// discoverSpecs scans searchRoot for *.wanda.yaml files and builds a name index.
334-
// Names are expanded using the provided lookup function.
332+
// Names are expanded using declared params first, then the lookup function.
333+
// Specs with params will have all param combinations indexed.
335334
// Returns an error if two specs expand to the same name.
336335
func discoverSpecs(searchRoot string, lookup lookupFunc) (specIndex, error) {
337336
index := make(specIndex)
@@ -356,13 +355,25 @@ func discoverSpecs(searchRoot string, lookup lookupFunc) (specIndex, error) {
356355
outCh <- discovered{skipped: true}
357356
continue
358357
}
359-
spec = spec.expandVar(lookup)
360358

361-
if vars := findUnexpandedVars(spec.Name); len(vars) > 0 {
359+
// Use params to enumerate all possible names.
360+
// For vars without params, $VARNAME is preserved.
361+
names := spec.ExpandedNames()
362+
sentAny := false
363+
for _, name := range names {
364+
// If still has unexpanded vars, try env lookup
365+
if vars := findUnexpandedVars(name); len(vars) > 0 {
366+
name = expandVar(name, lookup)
367+
if vars := findUnexpandedVars(name); len(vars) > 0 {
368+
continue // Can't fully expand, skip this name
369+
}
370+
}
371+
outCh <- discovered{name: name, path: path}
372+
sentAny = true
373+
}
374+
if !sentAny {
362375
outCh <- discovered{skipped: true}
363-
continue
364376
}
365-
outCh <- discovered{name: spec.Name, path: path}
366377
}
367378
}()
368379
}

wanda/deps_test.go

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -515,6 +515,70 @@ func TestDiscoverSpecs_WithVariables(t *testing.T) {
515515
}
516516
}
517517

518+
func TestDiscoverSpecs_WithParams(t *testing.T) {
519+
tmpDir := t.TempDir()
520+
521+
// Spec with params - should be indexed under all expanded names
522+
writeSpec(t, tmpDir, "base.wanda.yaml", strings.Join([]string{
523+
"name: base$PY",
524+
"params:",
525+
" PY:",
526+
" - '3.10'",
527+
" - '3.11'",
528+
" - '3.12'",
529+
"dockerfile: Dockerfile",
530+
}, "\n"))
531+
532+
// No env vars needed - params provide the values
533+
index, err := discoverSpecs(tmpDir, noopLookup)
534+
if err != nil {
535+
t.Fatalf("discoverSpecs: %v", err)
536+
}
537+
538+
// All three expanded names should be indexed
539+
for _, name := range []string{"base3.10", "base3.11", "base3.12"} {
540+
if _, ok := index[name]; !ok {
541+
t.Errorf("index missing %q, got: %v", name, index)
542+
}
543+
}
544+
545+
// All should point to the same spec file
546+
path := index["base3.10"]
547+
if index["base3.11"] != path || index["base3.12"] != path {
548+
t.Errorf("all names should map to same path, got: %v", index)
549+
}
550+
}
551+
552+
func TestDiscoverSpecs_ParamsAndEnvFallback(t *testing.T) {
553+
tmpDir := t.TempDir()
554+
555+
// Spec with partial params - one var has params, one needs env
556+
writeSpec(t, tmpDir, "base.wanda.yaml", strings.Join([]string{
557+
"name: base$PY-$ARCH",
558+
"params:",
559+
" PY:",
560+
" - '3.10'",
561+
"dockerfile: Dockerfile",
562+
}, "\n"))
563+
564+
lookup := func(key string) (string, bool) {
565+
if key == "ARCH" {
566+
return "amd64", true
567+
}
568+
return "", false
569+
}
570+
571+
index, err := discoverSpecs(tmpDir, lookup)
572+
if err != nil {
573+
t.Fatalf("discoverSpecs: %v", err)
574+
}
575+
576+
// Should be indexed as base3.10-amd64
577+
if _, ok := index["base3.10-amd64"]; !ok {
578+
t.Errorf("index missing base3.10-amd64, got: %v", index)
579+
}
580+
}
581+
518582
func TestBuildDepGraph_Discovery(t *testing.T) {
519583
tmpDir := t.TempDir()
520584

@@ -643,3 +707,133 @@ func TestBuildDepGraph_TransitiveDeps(t *testing.T) {
643707
t.Error("expected c in graph (transitive dep)")
644708
}
645709
}
710+
711+
func TestBuildDepGraph_ParamsValidation(t *testing.T) {
712+
tmpDir := t.TempDir()
713+
714+
writeSpec(t, tmpDir, "spec.wanda.yaml", strings.Join([]string{
715+
"name: myimage$PY_VERSION",
716+
"params:",
717+
" PY_VERSION:",
718+
" - '3.10'",
719+
" - '3.11'",
720+
"dockerfile: Dockerfile",
721+
}, "\n"))
722+
723+
t.Run("valid param value", func(t *testing.T) {
724+
lookup := func(k string) (string, bool) {
725+
if k == "PY_VERSION" {
726+
return "3.10", true
727+
}
728+
return "", false
729+
}
730+
_, err := buildDepGraph(filepath.Join(tmpDir, "spec.wanda.yaml"), lookup, "")
731+
if err != nil {
732+
t.Errorf("unexpected error with valid param: %v", err)
733+
}
734+
})
735+
736+
t.Run("invalid param value", func(t *testing.T) {
737+
lookup := func(k string) (string, bool) {
738+
if k == "PY_VERSION" {
739+
return "3.9", true
740+
}
741+
return "", false
742+
}
743+
_, err := buildDepGraph(filepath.Join(tmpDir, "spec.wanda.yaml"), lookup, "")
744+
if err == nil {
745+
t.Error("expected error for invalid param value")
746+
}
747+
if !strings.Contains(err.Error(), "3.9") {
748+
t.Errorf("error should mention invalid value '3.9': %v", err)
749+
}
750+
})
751+
}
752+
753+
func TestBuildDepGraph_UnexpandedWithParamsHint(t *testing.T) {
754+
tmpDir := t.TempDir()
755+
756+
// Spec with params but env var not set
757+
writeSpec(t, tmpDir, "spec.wanda.yaml", strings.Join([]string{
758+
"name: myimage$PY_VERSION",
759+
"params:",
760+
" PY_VERSION:",
761+
" - '3.10'",
762+
" - '3.11'",
763+
"dockerfile: Dockerfile",
764+
}, "\n"))
765+
766+
// No env var set - should get helpful error with valid values
767+
_, err := buildDepGraph(filepath.Join(tmpDir, "spec.wanda.yaml"), noopLookup, "")
768+
if err == nil {
769+
t.Fatal("expected error for unexpanded env var")
770+
}
771+
772+
// Error should mention valid values from params
773+
errStr := err.Error()
774+
if !strings.Contains(errStr, "PY_VERSION") {
775+
t.Errorf("error should mention PY_VERSION: %v", err)
776+
}
777+
if !strings.Contains(errStr, "valid values") {
778+
t.Errorf("error should mention valid values: %v", err)
779+
}
780+
if !strings.Contains(errStr, "3.10") || !strings.Contains(errStr, "3.11") {
781+
t.Errorf("error should list valid values 3.10, 3.11: %v", err)
782+
}
783+
}
784+
785+
func TestBuildDepGraph_DiscoveryWithParams(t *testing.T) {
786+
tmpDir := t.TempDir()
787+
788+
if err := os.Mkdir(filepath.Join(tmpDir, ".git"), 0755); err != nil {
789+
t.Fatal(err)
790+
}
791+
792+
// Base spec with params - discoverable via params, loadable with env var
793+
baseDir := filepath.Join(tmpDir, "base")
794+
if err := os.MkdirAll(baseDir, 0755); err != nil {
795+
t.Fatal(err)
796+
}
797+
writeSpec(t, baseDir, "base.wanda.yaml", strings.Join([]string{
798+
"name: base$PY",
799+
"params:",
800+
" PY:",
801+
" - '3.10'",
802+
" - '3.11'",
803+
"dockerfile: Dockerfile",
804+
}, "\n"))
805+
806+
// App spec depends on base3.10
807+
appDir := filepath.Join(tmpDir, "app")
808+
if err := os.MkdirAll(appDir, 0755); err != nil {
809+
t.Fatal(err)
810+
}
811+
writeSpec(t, appDir, "app.wanda.yaml", strings.Join([]string{
812+
"name: app",
813+
`froms: ["cr.ray.io/rayproject/base3.10"]`,
814+
"dockerfile: Dockerfile",
815+
}, "\n"))
816+
817+
// Discovery finds base3.10 via params (no env var needed for discovery).
818+
// Loading the spec requires env var to be set for expansion.
819+
lookup := func(key string) (string, bool) {
820+
if key == "PY" {
821+
return "3.10", true
822+
}
823+
return "", false
824+
}
825+
826+
graph, err := buildDepGraph(filepath.Join(appDir, "app.wanda.yaml"), lookup, testPrefix)
827+
if err != nil {
828+
t.Fatalf("buildDepGraph: %v", err)
829+
}
830+
831+
// base3.10 was discovered via params and loaded with PY=3.10
832+
if graph.Specs["base3.10"] == nil {
833+
t.Error("expected base3.10 in graph")
834+
}
835+
836+
if len(graph.Order) != 2 {
837+
t.Errorf("Order has %d items, want 2", len(graph.Order))
838+
}
839+
}

0 commit comments

Comments
 (0)