Merge pull request #1730 from ajayk/minor-cleanup

minor go cleanup and some performance improvements

Makefile

@@ -117,7 +117,7 @@ GOLANGCI_LINT_BIN = $(GOLANGCI_LINT_DIR)/golangci-lint
 setup-golangci-lint:
 	rm -f $(GOLANGCI_LINT_BIN) || :
 	set -e ;
-	GOBIN=$(GOLANGCI_LINT_DIR) go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.59.0;
+	GOBIN=$(GOLANGCI_LINT_DIR) go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.63.4;
 
 .PHONY: fmt
 fmt: ## Format all go files

pkg/build/pipeline.go

@@ -23,11 +23,10 @@ import (
 	"os/signal"
 	"path"
 	"path/filepath"
-	"regexp"
 	"strconv"
 	"strings"
 
-	apko_types "chainguard.dev/apko/pkg/build/types"
+	apkoTypes "chainguard.dev/apko/pkg/build/types"
 	"chainguard.dev/melange/pkg/cond"
 	"chainguard.dev/melange/pkg/config"
 	"chainguard.dev/melange/pkg/container"
@@ -74,7 +73,7 @@ func (sm *SubstitutionMap) Subpackage(subpkg *config.Subpackage) *SubstitutionMa
 	return &SubstitutionMap{nw}
 }
 
-func NewSubstitutionMap(cfg *config.Configuration, arch apko_types.Architecture, flavor string, buildOpts []string) (*SubstitutionMap, error) {
+func NewSubstitutionMap(cfg *config.Configuration, arch apkoTypes.Architecture, flavor string, buildOpts []string) (*SubstitutionMap, error) {
 	pkg := cfg.Package
 
 	nw := map[string]string{
@@ -139,47 +138,39 @@ func validateWith(data map[string]string, inputs map[string]config.Input) (map[s
 	if data == nil {
 		data = make(map[string]string)
 	}
-
 	for k, v := range inputs {
 		if data[k] == "" {
 			data[k] = v.Default
 		}
-		if k == "expected-sha256" && data[k] != "" {
-			if !matchValidShaChars(data[k]) {
-				return data, fmt.Errorf("checksum input %q for pipeline contains invalid characters", k)
-			}
-			if len(data[k]) != 64 {
-				return data, fmt.Errorf("checksum input %q for pipeline, invalid length", k)
-			}
-		}
-		if k == "expected-sha512" && data[k] != "" {
-			if !matchValidShaChars(data[k]) {
-				return data, fmt.Errorf("checksum input %q for pipeline contains invalid characters", k)
-			}
-			if len(data[k]) != 128 {
-				return data, fmt.Errorf("checksum input %q for pipeline, invalid length", k)
-			}
-		}
-		if k == "expected-commit" && data[k] != "" {
-			if !matchValidShaChars(data[k]) {
-				return data, fmt.Errorf("expectec commit %q for pipeline contains invalid characters", k)
-			}
-			if len(data[k]) != 40 {
-				return data, fmt.Errorf("expected commit %q for pipeline, invalid length", k)
+		if data[k] != "" {
+			switch k {
+			case "expected-sha256", "expected-sha512":
+				if !matchValidShaChars(data[k]) || len(data[k]) != expectedShaLength(k) {
+					return data, fmt.Errorf("checksum input %q for pipeline, invalid length", k)
+				}
+			case "expected-commit":
+				if !matchValidShaChars(data[k]) || len(data[k]) != expectedShaLength(k) {
+					return data, fmt.Errorf("expected commit %q for pipeline contains invalid characters or invalid sha length", k)
+				}
 			}
 		}
-
 		if v.Required && data[k] == "" {
 			return data, fmt.Errorf("required input %q for pipeline is missing", k)
 		}
+
 	}
 
 	return data, nil
 }
 
 func matchValidShaChars(s string) bool {
-	match, _ := regexp.MatchString("^[a-fA-F0-9]+$", s)
-	return match
+	for i := 0; i < len(s); i++ {
+		c := s[i]
+		if !(c >= '0' && c <= '9') && !(c >= 'a' && c <= 'f') && !(c >= 'A' && c <= 'F') {
+			return false
+		}
+	}
+	return true
 }
 
 // Build a script to run as part of evalRun
@@ -335,5 +326,17 @@ func shouldRun(ifs string) (bool, error) {
 	return result, nil
 }
 
+func expectedShaLength(shaType string) int {
+	switch shaType {
+	case "expected-sha256":
+		return 64
+	case "expected-sha512":
+		return 128
+	case "expected-commit":
+		return 40
+	}
+	return 0
+}
+
 //go:embed pipelines/*
 var f embed.FS

pkg/build/pipeline_test.go

@@ -172,3 +172,63 @@ func TestAllPipelines(t *testing.T) {
 		})
 	}
 }
+
+func Test_validateWith(t *testing.T) {
+	tests := []struct {
+		name        string
+		data        map[string]string
+		inputs      map[string]config.Input
+		expected    map[string]string
+		expectError bool
+		errorMsg    string
+	}{
+		{
+			name: "Valid SHA256 checksum",
+			data: map[string]string{
+				"expected-sha256": "a3c2567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef",
+			},
+			inputs: map[string]config.Input{
+				"expected-sha256": {Default: "", Required: true},
+			},
+			expected: map[string]string{
+				"expected-sha256": "a3c2567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef",
+			},
+			expectError: false,
+		},
+		{
+			name: "Invalid SHA256 length",
+			data: map[string]string{
+				"expected-sha256": "abcdef",
+			},
+			inputs: map[string]config.Input{
+				"expected-sha256": {Default: "", Required: true},
+			},
+			expectError: true,
+			errorMsg:    "checksum input \"expected-sha256\" for pipeline, invalid length",
+		},
+		{
+			name: "Missing required input",
+			data: map[string]string{},
+			inputs: map[string]config.Input{
+				"expected-commit": {Default: "", Required: true},
+			},
+			expectError: true,
+			errorMsg:    "required input \"expected-commit\" for pipeline is missing",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := validateWith(tt.data, tt.inputs)
+
+			if tt.expectError {
+				require.Error(t, err)
+				require.EqualError(t, err, tt.errorMsg)
+				return // Skip further checks if error is expected
+			}
+
+			require.NoError(t, err)
+			require.Equal(t, tt.expected, result)
+		})
+	}
+}

pkg/convert/relmon/release_monitoring.go

@@ -41,7 +41,7 @@ type MonitorFinder struct {
 func (mf *MonitorFinder) FindMonitor(ctx context.Context, pkg string) (*Item, error) {
 	var items *Items
 	url := fmt.Sprintf(searchFmt, pkg)
-	req, err := http.NewRequestWithContext(ctx, "GET", url, nil)
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
 	if err != nil {
 		return nil, err
 	}

pkg/http/http.go

@@ -61,17 +61,12 @@ func (c *RLHTTPClient) GetArtifactSHA256(ctx context.Context, artifactURI string
 	}
 
 	defer resp.Body.Close()
-
 	if resp.StatusCode != http.StatusOK {
 		return "", fmt.Errorf("%d when getting %s", resp.StatusCode, artifactURI)
 	}
-
-	body, err := io.ReadAll(resp.Body)
-	if err != nil {
-		return "", fmt.Errorf("reading body: %w", err)
-	}
-
 	h256 := sha256.New()
-	h256.Write(body)
+	if _, err := io.Copy(h256, resp.Body); err != nil {
+		return "", fmt.Errorf("hashing %s: %w", artifactURI, err)
+	}
 	return fmt.Sprintf("%x", h256.Sum(nil)), nil
 }

pkg/renovate/cache/cache.go

@@ -222,7 +222,7 @@ func addFileToCache(ctx context.Context, cfg CacheConfig, downloadedFile string,
 func downloadFile(ctx context.Context, uri string) (string, error) {
 	targetFile, err := os.CreateTemp("", "melange-update-*")
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("failed to create temp file: %w", err)
 	}
 	defer targetFile.Close()
 
@@ -234,23 +234,23 @@ func downloadFile(ctx context.Context, uri string) (string, error) {
 		},
 	}
 
-	req, err := http.NewRequestWithContext(ctx, "GET", uri, nil)
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, uri, nil)
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("failed to create HTTP request: %w", err)
 	}
 
 	// Set accept header to match the expected MIME types and avoid 403's for some servers like https://www.netfilter.org
 	req.Header.Set("Accept", "text/html")
 
 	resp, err := client.Do(req)
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("failed to fetch URL %s: %w", uri, err)
 	}
 
 	defer resp.Body.Close()
 
-	if resp.StatusCode != 200 {
-		return "", fmt.Errorf("got %s when fetching %s", resp.Status, uri)
+	if resp.StatusCode != http.StatusOK {
+		return "", fmt.Errorf("unexpected status code %d (%s) when fetching %s", resp.StatusCode, resp.Status, uri)
 	}
 
 	if _, err := io.Copy(targetFile, resp.Body); err != nil {

pkg/sbom/package.go

@@ -20,7 +20,6 @@ package sbom
 import (
 	"context"
 	"fmt"
-	"regexp"
 	"sort"
 	"strconv"
 	"strings"
@@ -174,10 +173,6 @@ func (p Package) getExternalRefs() []spdx.ExternalRef {
 	return result
 }
 
-// invalidIDCharsRe is a regular expression that matches characters not
-// considered valid in SPDX identifiers.
-var invalidIDCharsRe = regexp.MustCompile(`[^a-zA-Z0-9-.]+`)
-
 // stringToIdentifier converts a string to a valid SPDX identifier by replacing
 // invalid characters. Colons and slashes are replaced by dashes, and all other
 // invalid characters are replaced by their Unicode code point prefixed with
@@ -189,20 +184,21 @@ var invalidIDCharsRe = regexp.MustCompile(`[^a-zA-Z0-9-.]+`)
 //	"foo/bar" -> "foo-bar"
 //	"foo bar" -> "fooC32bar"
 func stringToIdentifier(in string) string {
-	in = strings.ReplaceAll(in, ":", "-")
-	in = strings.ReplaceAll(in, "/", "-")
-
-	invalidCharReplacer := func(s string) string {
-		sb := strings.Builder{}
-		for _, r := range s {
+	var sb strings.Builder
+	sb.Grow(len(in))
+
+	for _, r := range in {
+		switch {
+		case r == ':' || r == '/':
+			sb.WriteRune('-')
+		case r == '-' || r == '.' || (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9'):
+			sb.WriteRune(r)
+		default:
 			sb.WriteString(encodeInvalidRune(r))
 		}
-		return sb.String()
 	}
-
-	return invalidIDCharsRe.ReplaceAllStringFunc(in, invalidCharReplacer)
+	return sb.String()
 }
-
 func encodeInvalidRune(r rune) string {
 	return "C" + strconv.Itoa(int(r))
 }

pkg/sbom/package_test.go

@@ -0,0 +1,61 @@
+// Copyright 2024 Chainguard, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package sbom
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func Test_stringToIdentifier(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name:     "basic_colon",
+			input:    "foo:bar",
+			expected: "foo-bar", // Colons replaced with dashes.
+		},
+		{
+			name:     "basic_slash",
+			input:    "foo/bar",
+			expected: "foo-bar", // Slashes replaced with dashes.
+		},
+		{
+			name:     "space_replacement",
+			input:    "foo bar",
+			expected: "fooC32bar", // Spaces encoded as Unicode prefix.
+		},
+		{
+			name:     "mixed_colon_and_slash",
+			input:    "foo:bar/baz",
+			expected: "foo-bar-baz", // Mixed colons and slashes replaced with dashes.
+		},
+		{
+			name:     "valid_characters_unchanged",
+			input:    "example-valid.123",
+			expected: "example-valid.123", // Valid characters remain unchanged.
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			result := stringToIdentifier(test.input)
+			require.Equal(t, test.expected, result, "unexpected result for input %q", test.input)
+		})
+	}
+}