Align chart name validation with create

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

internal/chart/v3/lint/rules/chartfile.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"regexp"
 
 	"github.com/Masterminds/semver/v3"
 	"github.com/asaskevich/govalidator"
@@ -32,10 +31,6 @@ import (
 	chartutil "helm.sh/helm/v4/internal/chart/v3/util"
 )
 
-const validChartNameMessage = "chart names must contain lowercase letters, numbers, and dashes, and must start and end with a lowercase letter or number"
-
-var validChartName = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
-
 // Chartfile runs a set of linter rules related to Chart.yaml file
 func Chartfile(linter *support.Linter) {
 	chartFileName := "Chart.yaml"
@@ -125,8 +120,8 @@ func validateChartName(cf *chart.Metadata) error {
 	if name != cf.Name {
 		return fmt.Errorf("chart name %q is invalid", cf.Name)
 	}
-	if !validChartName.MatchString(cf.Name) {
-		return fmt.Errorf("chart name %q is invalid; %s", cf.Name, validChartNameMessage)
+	if err := chartutil.ValidateChartName(cf.Name); err != nil {
+		return fmt.Errorf("chart name %q is invalid; %w", cf.Name, err)
 	}
 	return nil
 }

internal/chart/v3/lint/rules/chartfile_test.go

@@ -64,6 +64,8 @@ func TestValidateChartYamlFormat(t *testing.T) {
 }
 
 func TestValidateChartName(t *testing.T) {
+	const chartNameRule = "chart names must contain lowercase letters, numbers, and dashes, and must start and end with a lowercase letter or number"
+
 	tests := []struct {
 		name    string
 		wantErr string
@@ -78,7 +80,7 @@ func TestValidateChartName(t *testing.T) {
 		},
 		{
 			name:    "BadChart",
-			wantErr: `chart name "BadChart" is invalid`,
+			wantErr: `chart name "BadChart" is invalid; ` + chartNameRule,
 		},
 		{
 			name:    "bad.chart",

internal/chart/v3/util/create.go

@@ -17,6 +17,7 @@ limitations under the License.
 package util
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -32,10 +33,9 @@ import (
 )
 
 // chartName is a regular expression for testing the supplied name of a chart.
-// This regular expression is probably stricter than it needs to be. We can relax it
-// somewhat. Newline characters, as well as $, quotes, +, parens, and % are known to be
-// problematic.
-var chartName = regexp.MustCompile("^[a-zA-Z0-9._-]+$")
+var chartName = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
+
+const validChartNameMessage = "chart names must contain lowercase letters, numbers, and dashes, and must start and end with a lowercase letter or number"
 
 const (
 	// ChartfileName is the default Chart file name.
@@ -650,6 +650,10 @@ var Stderr io.Writer = os.Stderr
 
 // CreateFrom creates a new chart, but scaffolds it from the src chart.
 func CreateFrom(chartfile *chart.Metadata, dest, src string) error {
+	if err := ValidateChartName(chartfile.Name); err != nil {
+		return err
+	}
+
 	schart, err := loader.Load(src)
 	if err != nil {
 		return fmt.Errorf("could not load %s: %w", src, err)
@@ -704,7 +708,7 @@ func CreateFrom(chartfile *chart.Metadata, dest, src string) error {
 func Create(name, dir string) (string, error) {
 
 	// Sanity-check the name of a chart so user doesn't create one that causes problems.
-	if err := validateChartName(name); err != nil {
+	if err := ValidateChartName(name); err != nil {
 		return "", err
 	}
 
@@ -823,12 +827,13 @@ func writeFile(name string, content []byte) error {
 	return os.WriteFile(name, content, 0644)
 }
 
-func validateChartName(name string) error {
+// ValidateChartName validates a chart name used by chart creation and linting.
+func ValidateChartName(name string) error {
 	if name == "" || len(name) > maxChartNameLength {
 		return fmt.Errorf("chart name must be between 1 and %d characters", maxChartNameLength)
 	}
 	if !chartName.MatchString(name) {
-		return fmt.Errorf("chart name must match the regular expression %q", chartName.String())
+		return errors.New(validChartNameMessage)
 	}
 	return nil
 }

internal/chart/v3/util/create_test.go

@@ -107,6 +107,13 @@ func TestCreateFrom(t *testing.T) {
 			t.Errorf("File %s contains <CHARTNAME>", f)
 		}
 	}
+
+	cf.Name = "Bad.Chart"
+	if err := CreateFrom(cf, t.TempDir(), srcdir); err == nil {
+		t.Fatal("CreateFrom with invalid chart name returned no error")
+	} else if err.Error() != validChartNameMessage {
+		t.Fatalf("CreateFrom returned %q, want %q", err, validChartNameMessage)
+	}
 }
 
 // TestCreate_Overwrite is a regression test for making sure that files are overwritten.
@@ -145,28 +152,89 @@ func TestCreate_Overwrite(t *testing.T) {
 }
 
 func TestValidateChartName(t *testing.T) {
-	for name, shouldPass := range map[string]bool{
-		"":                              false,
-		"abcdefghijklmnopqrstuvwxyz-_.": true,
-		"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.": true,
-		"$hello":                        false,
-		"Hellô":                         false,
-		"he%%o":                         false,
-		"he\nllo":                       false,
+	tests := []struct {
+		name    string
+		wantErr string
+	}{
+		{
+			name:    "",
+			wantErr: "chart name must be between 1 and 250 characters",
+		},
+		{
+			name: "abcdefghijklmnopqrstuvwxyz",
+		},
+		{
+			name: "bad-chart-1",
+		},
+		{
+			name: "bad--chart",
+		},
+		{
+			name:    "BadChart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "bad.chart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "bad_chart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "-badchart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "badchart-",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "$hello",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "Hellô",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "he%%o",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "he\nllo",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name: "abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.",
+			wantErr: "chart name must be between 1 and 250 characters",
+		},
+	}
 
-		"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.": false,
-	} {
-		if err := validateChartName(name); (err != nil) == shouldPass {
-			t.Errorf("test for %q failed", name)
-		}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := ValidateChartName(tt.name)
+			if tt.wantErr == "" {
+				if err != nil {
+					t.Fatalf("ValidateChartName(%q) returned unexpected error: %s", tt.name, err)
+				}
+				return
+			}
+			if err == nil {
+				t.Fatalf("ValidateChartName(%q) returned no error, want %q", tt.name, tt.wantErr)
+			}
+			if err.Error() != tt.wantErr {
+				t.Fatalf("ValidateChartName(%q) returned %q, want %q", tt.name, err, tt.wantErr)
+			}
+		})
 	}
 }

pkg/chart/v2/lint/rules/chartfile.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"regexp"
 
 	"github.com/Masterminds/semver/v3"
 	"github.com/asaskevich/govalidator"
@@ -32,10 +31,6 @@ import (
 	chartutil "helm.sh/helm/v4/pkg/chart/v2/util"
 )
 
-const validChartNameMessage = "chart names must contain lowercase letters, numbers, and dashes, and must start and end with a lowercase letter or number"
-
-var validChartName = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
-
 // Chartfile runs a set of linter rules related to Chart.yaml file
 func Chartfile(linter *support.Linter) {
 	chartFileName := "Chart.yaml"
@@ -126,8 +121,8 @@ func validateChartName(cf *chart.Metadata) error {
 	if name != cf.Name {
 		return fmt.Errorf("chart name %q is invalid", cf.Name)
 	}
-	if !validChartName.MatchString(cf.Name) {
-		return fmt.Errorf("chart name %q is invalid; %s", cf.Name, validChartNameMessage)
+	if err := chartutil.ValidateChartName(cf.Name); err != nil {
+		return fmt.Errorf("chart name %q is invalid; %w", cf.Name, err)
 	}
 	return nil
 }

pkg/chart/v2/lint/rules/chartfile_test.go

@@ -64,6 +64,8 @@ func TestValidateChartYamlFormat(t *testing.T) {
 }
 
 func TestValidateChartName(t *testing.T) {
+	const chartNameRule = "chart names must contain lowercase letters, numbers, and dashes, and must start and end with a lowercase letter or number"
+
 	tests := []struct {
 		name    string
 		wantErr string
@@ -78,7 +80,7 @@ func TestValidateChartName(t *testing.T) {
 		},
 		{
 			name:    "BadChart",
-			wantErr: `chart name "BadChart" is invalid`,
+			wantErr: `chart name "BadChart" is invalid; ` + chartNameRule,
 		},
 		{
 			name:    "bad.chart",

pkg/chart/v2/util/create.go

@@ -17,6 +17,7 @@ limitations under the License.
 package util
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -32,10 +33,9 @@ import (
 )
 
 // chartName is a regular expression for testing the supplied name of a chart.
-// This regular expression is probably stricter than it needs to be. We can relax it
-// somewhat. Newline characters, as well as $, quotes, +, parens, and % are known to be
-// problematic.
-var chartName = regexp.MustCompile("^[a-zA-Z0-9._-]+$")
+var chartName = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
+
+const validChartNameMessage = "chart names must contain lowercase letters, numbers, and dashes, and must start and end with a lowercase letter or number"
 
 const (
 	// ChartfileName is the default Chart file name.
@@ -649,6 +649,10 @@ var Stderr io.Writer = os.Stderr
 
 // CreateFrom creates a new chart, but scaffolds it from the src chart.
 func CreateFrom(chartfile *chart.Metadata, dest, src string) error {
+	if err := ValidateChartName(chartfile.Name); err != nil {
+		return err
+	}
+
 	schart, err := loader.Load(src)
 	if err != nil {
 		return fmt.Errorf("could not load %s: %w", src, err)
@@ -703,7 +707,7 @@ func CreateFrom(chartfile *chart.Metadata, dest, src string) error {
 func Create(name, dir string) (string, error) {
 
 	// Sanity-check the name of a chart so user doesn't create one that causes problems.
-	if err := validateChartName(name); err != nil {
+	if err := ValidateChartName(name); err != nil {
 		return "", err
 	}
 
@@ -822,12 +826,13 @@ func writeFile(name string, content []byte) error {
 	return os.WriteFile(name, content, 0644)
 }
 
-func validateChartName(name string) error {
+// ValidateChartName validates a chart name used by chart creation and linting.
+func ValidateChartName(name string) error {
 	if name == "" || len(name) > maxChartNameLength {
 		return fmt.Errorf("chart name must be between 1 and %d characters", maxChartNameLength)
 	}
 	if !chartName.MatchString(name) {
-		return fmt.Errorf("chart name must match the regular expression %q", chartName.String())
+		return errors.New(validChartNameMessage)
 	}
 	return nil
 }

pkg/chart/v2/util/create_test.go

@@ -107,6 +107,13 @@ func TestCreateFrom(t *testing.T) {
 			t.Errorf("File %s contains <CHARTNAME>", f)
 		}
 	}
+
+	cf.Name = "Bad.Chart"
+	if err := CreateFrom(cf, t.TempDir(), srcdir); err == nil {
+		t.Fatal("CreateFrom with invalid chart name returned no error")
+	} else if err.Error() != validChartNameMessage {
+		t.Fatalf("CreateFrom returned %q, want %q", err, validChartNameMessage)
+	}
 }
 
 // TestCreate_Overwrite is a regression test for making sure that files are overwritten.
@@ -145,28 +152,89 @@ func TestCreate_Overwrite(t *testing.T) {
 }
 
 func TestValidateChartName(t *testing.T) {
-	for name, shouldPass := range map[string]bool{
-		"":                              false,
-		"abcdefghijklmnopqrstuvwxyz-_.": true,
-		"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.": true,
-		"$hello":                        false,
-		"Hellô":                         false,
-		"he%%o":                         false,
-		"he\nllo":                       false,
+	tests := []struct {
+		name    string
+		wantErr string
+	}{
+		{
+			name:    "",
+			wantErr: "chart name must be between 1 and 250 characters",
+		},
+		{
+			name: "abcdefghijklmnopqrstuvwxyz",
+		},
+		{
+			name: "bad-chart-1",
+		},
+		{
+			name: "bad--chart",
+		},
+		{
+			name:    "BadChart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "bad.chart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "bad_chart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "-badchart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "badchart-",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "$hello",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "Hellô",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "he%%o",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "he\nllo",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name: "abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.",
+			wantErr: "chart name must be between 1 and 250 characters",
+		},
+	}
 
-		"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.": false,
-	} {
-		if err := validateChartName(name); (err != nil) == shouldPass {
-			t.Errorf("test for %q failed", name)
-		}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := ValidateChartName(tt.name)
+			if tt.wantErr == "" {
+				if err != nil {
+					t.Fatalf("ValidateChartName(%q) returned unexpected error: %s", tt.name, err)
+				}
+				return
+			}
+			if err == nil {
+				t.Fatalf("ValidateChartName(%q) returned no error, want %q", tt.name, tt.wantErr)
+			}
+			if err.Error() != tt.wantErr {
+				t.Fatalf("ValidateChartName(%q) returned %q, want %q", tt.name, err, tt.wantErr)
+			}
+		})
 	}
 }