addressed review comments

Author: arodage

internal/config/config.go

@@ -99,7 +99,7 @@ func parseYAMLTemplate(data []byte) (*ImageTemplate, error) {
 	}
 
 	// Validate against image template schema
-	if err := validate.ValidateImageJSON(jsonData); err != nil {
+	if err := validate.ValidateImageTemplateJSON(jsonData); err != nil {
 		return nil, fmt.Errorf("template validation error: %w", err)
 	}
 
@@ -109,36 +109,9 @@ func parseYAMLTemplate(data []byte) (*ImageTemplate, error) {
 		return nil, fmt.Errorf("parsing template: %w", err)
 	}
 
-	// Validate template has required content
-	if err := validateTemplateContent(&template); err != nil {
-		return nil, fmt.Errorf("template content validation: %w", err)
-	}
-
 	return &template, nil
 }
 
-// validateTemplateContent validates the template has required content
-func validateTemplateContent(template *ImageTemplate) error {
-	if len(template.SystemConfigs) == 0 {
-		return fmt.Errorf("no system configurations found in template")
-	}
-
-	// Validate supported OS combinations
-	validCombinations := map[string]map[string]bool{
-		"azure-linux": {"azl3": true},
-		"emt":         {"emt3": true},
-		"elxr":        {"elxr12": true},
-	}
-
-	if validDists, ok := validCombinations[template.Target.OS]; !ok {
-		return fmt.Errorf("unsupported OS: %s", template.Target.OS)
-	} else if !validDists[template.Target.Dist] {
-		return fmt.Errorf("unsupported distribution %s for OS %s", template.Target.Dist, template.Target.OS)
-	}
-
-	return nil
-}
-
 // GetProviderName returns the provider name for the given template
 func (t *ImageTemplate) GetProviderName() string {
 	// Map OS/dist combinations to provider names
@@ -197,7 +170,7 @@ func DefaultGlobalConfig() *GlobalConfig {
 		Workers:  8,
 		CacheDir: "./cache",
 		WorkDir:  "./workspace",
-		TempDir:  "",
+		TempDir:  "./tmp",
 
 		Logging: LoggingConfig{
 			Level: "info",

internal/config/config_test.go

@@ -158,56 +158,51 @@ func TestUnsupportedFileFormat(t *testing.T) {
 }
 
 func TestUnsupportedDistribution(t *testing.T) {
-	template := &ImageTemplate{
-		Target: struct {
-			OS        string `yaml:"os"`
-			Dist      string `yaml:"dist"`
-			Arch      string `yaml:"arch"`
-			ImageType string `yaml:"imageType"`
-		}{
-			OS:        "unknown-os",
-			Dist:      "unknown-dist",
-			Arch:      "x86_64",
-			ImageType: "iso",
-		},
-		SystemConfigs: []SystemConfig{
-			{
-				Name:        "test-config",
-				Description: "Test configuration",
-				Packages:    []string{"package1"},
-				Kernel: KernelConfig{
-					Version: "6.12",
-					Cmdline: "quiet",
-				},
-			},
-		},
-	}
+	// This test verifies that schema validation catches invalid OS/dist combinations
 
-	err := validateTemplateContent(template)
+	invalidYaml := `image:
+  name: test
+  version: "1.0.0"
+target:
+  os: azure-linux
+  dist: emt3  # Invalid: azure-linux should only use azl3
+  arch: x86_64
+  imageType: raw
+systemConfigs:
+  - name: test
+    packages: ["test"]
+    kernel:
+      version: "6.12"
+`
+
+	// This should fail during schema validation
+	_, err := parseYAMLTemplate([]byte(invalidYaml))
 	if err == nil {
-		t.Errorf("expected error for unsupported OS")
+		t.Errorf("expected schema validation error for invalid OS/dist combination")
+	}
+	if !strings.Contains(err.Error(), "template validation error") {
+		t.Errorf("expected schema validation error, got: %v", err)
 	}
 }
 
 func TestEmptySystemConfigs(t *testing.T) {
-	template := &ImageTemplate{
-		Target: struct {
-			OS        string `yaml:"os"`
-			Dist      string `yaml:"dist"`
-			Arch      string `yaml:"arch"`
-			ImageType string `yaml:"imageType"`
-		}{
-			OS:        "azure-linux",
-			Dist:      "azl3",
-			Arch:      "x86_64",
-			ImageType: "iso",
-		},
-		SystemConfigs: []SystemConfig{}, // Empty
-	}
+	// This test verifies that schema validation catches empty systemConfigs
 
-	err := validateTemplateContent(template)
+	invalidYaml := `image:
+  name: test
+  version: "1.0.0"
+target:
+  os: azure-linux
+  dist: azl3
+  arch: x86_64
+  imageType: raw
+systemConfigs: []  # Invalid: minItems is 1
+`
+
+	// This should fail during schema validation
+	_, err := parseYAMLTemplate([]byte(invalidYaml))
 	if err == nil {
-		t.Errorf("expected error for empty system configurations")
+		t.Errorf("expected schema validation error for empty systemConfigs")
 	}
 }
 
@@ -256,11 +251,5 @@ func TestAllSupportedProviders(t *testing.T) {
 		if version != tc.version {
 			t.Errorf("for %s/%s expected version '%s', got '%s'", tc.os, tc.dist, tc.version, version)
 		}
-
-		// Test validation
-		err := validateTemplateContent(template)
-		if err != nil {
-			t.Errorf("validation failed for %s/%s: %v", tc.os, tc.dist, err)
-		}
 	}
 }

internal/validate/validate.go

@@ -33,20 +33,11 @@ func ValidateAgainstSchema(name string, schemaBytes, data []byte) error {
 	return nil
 }
 
-// ValidateComposerJSON runs the composer schema against data
-func ValidateComposerJSON(data []byte) error {
-	return ValidateAgainstSchema(
-		"os-image-composer.schema.json",
-		schema_pkg.ComposerSchema,
-		data,
-	)
-}
-
-// ValidateTemplateJSON runs the template schema against data
-func ValidateImageJSON(data []byte) error {
+// ValidateImageTemplateJSON runs the template schema against data
+func ValidateImageTemplateJSON(data []byte) error {
 	return ValidateAgainstSchema(
 		"os-image-template.schema.json",
-		schema_pkg.ImageSchema,
+		schema_pkg.ImageTemplateSchema,
 		data,
 	)
 }

internal/validate/validate_test.go

@@ -22,21 +22,6 @@ func loadFile(t *testing.T, relPath string) []byte {
 	return data
 }
 
-// Test legacy JSON composer format (for backward compatibility)
-func TestValidComposerJSON(t *testing.T) {
-	v := loadFile(t, "testdata/valid.json")
-	if err := ValidateComposerJSON(v); err != nil {
-		t.Errorf("expected valid.json to pass, but got: %v", err)
-	}
-}
-
-func TestInvalidComposerJSON(t *testing.T) {
-	v := loadFile(t, "testdata/invalid.json")
-	if err := ValidateComposerJSON(v); err == nil {
-		t.Errorf("expected invalid.json to fail validation")
-	}
-}
-
 // Test new YAML image template format
 func TestValidImageTemplate(t *testing.T) {
 	v := loadFile(t, "image-templates/azl3-x86_64-edge-raw.yml")
@@ -54,7 +39,7 @@ func TestValidImageTemplate(t *testing.T) {
 		t.Errorf("json marshaling error: %v", err)
 		return
 	}
-	if err := ValidateImageJSON(dataJSON); err != nil {
+	if err := ValidateImageTemplateJSON(dataJSON); err != nil {
 		t.Errorf("expected image-templates/azl3-x86_64-edge-raw.yml to pass, but got: %v", err)
 	}
 }
@@ -76,7 +61,7 @@ func TestInvalidImageTemplate(t *testing.T) {
 		return
 	}
 
-	if err := ValidateImageJSON(dataJSON); err == nil {
+	if err := ValidateImageTemplateJSON(dataJSON); err == nil {
 		t.Errorf("expected testdata/invalid-image.yml to pass, but got: %v", err)
 	}
 }
@@ -117,36 +102,15 @@ func TestInvalidConfig(t *testing.T) {
 
 	if err := ValidateConfigJSON(dataJSON); err == nil {
 		t.Errorf("expected invalid-config.json to fail validation: %v", err)
-	} else {
-		t.Logf("expected validation error: %v", err)
 	}
 }
 
-// Test validation of template structure
+// Test validation of template structure using external test files
 func TestImageTemplateStructure(t *testing.T) {
-	// Test a minimal valid template
-	minimalTemplate := `image:
-  name: test-image
-  version: "1.0.0"
-
-target:
-  os: azure-linux
-  dist: azl3
-  arch: x86_64
-  imageType: raw
-
-systemConfigs:
-  - name: minimal
-    description: Minimal test configuration
-    packages:
-      - openssh-server
-    kernel:
-      version: "6.12"
-      cmdline: "quiet"
-`
+	v := loadFile(t, "testdata/complete-valid-template.yml")
 
 	var raw interface{}
-	if err := yaml.Unmarshal([]byte(minimalTemplate), &raw); err != nil {
+	if err := yaml.Unmarshal(v, &raw); err != nil {
 		t.Fatalf("failed to parse minimal template: %v", err)
 	}
 
@@ -155,29 +119,16 @@ systemConfigs:
 		t.Fatalf("failed to marshal to JSON: %v", err)
 	}
 
-	if err := ValidateImageJSON(dataJSON); err != nil {
+	if err := ValidateImageTemplateJSON(dataJSON); err != nil {
 		t.Errorf("minimal template should be valid, but got: %v", err)
 	}
 }
 
 func TestImageTemplateMissingFields(t *testing.T) {
-	// Test template missing required fields
-	invalidTemplate := `image:
-  name: test-image
-
-target:
-  os: azure-linux
-  dist: azl3
-  arch: x86_64
-
-systemConfigs:
-  - name: incomplete
-    packages:
-      - openssh-server
-`
+	v := loadFile(t, "testdata/incomplete-template.yml")
 
 	var raw interface{}
-	if err := yaml.Unmarshal([]byte(invalidTemplate), &raw); err != nil {
+	if err := yaml.Unmarshal(v, &raw); err != nil {
 		t.Fatalf("failed to parse invalid template: %v", err)
 	}
 
@@ -186,7 +137,71 @@ systemConfigs:
 		t.Fatalf("failed to marshal to JSON: %v", err)
 	}
 
-	if err := ValidateImageJSON(dataJSON); err == nil {
+	if err := ValidateImageTemplateJSON(dataJSON); err == nil {
 		t.Errorf("incomplete template should fail validation")
 	}
 }
+
+// Table-driven test for multiple template validation scenarios
+func TestImageTemplateValidation(t *testing.T) {
+	tests := []struct {
+		name        string
+		file        string
+		shouldPass  bool
+		description string
+	}{
+		{
+			name:        "ValidComplete",
+			file:        "testdata/complete-valid-template.yml",
+			shouldPass:  true,
+			description: "complete template with all optional fields",
+		},
+		{
+			name:        "InvalidMissingImage",
+			file:        "testdata/missing-image-section.yml",
+			shouldPass:  false,
+			description: "template missing image section",
+		},
+		{
+			name:        "InvalidMissingTarget",
+			file:        "testdata/missing-target-section.yml",
+			shouldPass:  false,
+			description: "template missing target section",
+		},
+		{
+			name:        "InvalidMissingSystemConfigs",
+			file:        "testdata/missing-systemconfigs.yml",
+			shouldPass:  false,
+			description: "template missing systemConfigs section",
+		},
+		{
+			name:        "InvalidWrongTypes",
+			file:        "testdata/wrong-field-types.yml",
+			shouldPass:  false,
+			description: "template with incorrect field types",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			v := loadFile(t, tt.file)
+
+			var raw interface{}
+			if err := yaml.Unmarshal(v, &raw); err != nil {
+				t.Fatalf("failed to parse template %s: %v", tt.file, err)
+			}
+
+			dataJSON, err := json.Marshal(raw)
+			if err != nil {
+				t.Fatalf("failed to marshal to JSON: %v", err)
+			}
+
+			err = ValidateImageTemplateJSON(dataJSON)
+			if tt.shouldPass && err != nil {
+				t.Errorf("expected %s to pass validation (%s), but got error: %v", tt.file, tt.description, err)
+			} else if !tt.shouldPass && err == nil {
+				t.Errorf("expected %s to fail validation (%s), but it passed", tt.file, tt.description)
+			}
+		})
+	}
+}

schema/embed.go

@@ -2,11 +2,8 @@ package schema
 
 import _ "embed"
 
-//go:embed os-image-composer.schema.json
-var ComposerSchema []byte
-
 //go:embed os-image-template.schema.json
-var ImageSchema []byte
+var ImageTemplateSchema []byte
 
 //go:embed os-image-composer-config.schema.json
 var ConfigSchema []byte