Refactor codebase for testability and clean architecture

internal/files/interfaces.go

@@ -0,0 +1,57 @@
+// Copyright 2025 Stacklok, 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 files
+
+// VersionReader reads version information from files.
+type VersionReader interface {
+	// ReadVersion reads the version from the specified path.
+	ReadVersion(path string) (string, error)
+}
+
+// VersionWriter writes version information to files.
+type VersionWriter interface {
+	// WriteVersion writes the version to the specified path.
+	WriteVersion(path, version string) error
+}
+
+// YAMLUpdater updates version information in YAML files.
+type YAMLUpdater interface {
+	// UpdateYAMLFile updates a specific path in a YAML file with a new version.
+	UpdateYAMLFile(cfg VersionFileConfig, currentVersion, newVersion string) error
+}
+
+// DefaultVersionReader is the default implementation of VersionReader.
+type DefaultVersionReader struct{}
+
+// ReadVersion reads the version from the specified path.
+func (*DefaultVersionReader) ReadVersion(path string) (string, error) {
+	return ReadVersion(path)
+}
+
+// DefaultVersionWriter is the default implementation of VersionWriter.
+type DefaultVersionWriter struct{}
+
+// WriteVersion writes the version to the specified path.
+func (*DefaultVersionWriter) WriteVersion(path, version string) error {
+	return WriteVersion(path, version)
+}
+
+// DefaultYAMLUpdater is the default implementation of YAMLUpdater.
+type DefaultYAMLUpdater struct{}
+
+// UpdateYAMLFile updates a specific path in a YAML file with a new version.
+func (*DefaultYAMLUpdater) UpdateYAMLFile(cfg VersionFileConfig, currentVersion, newVersion string) error {
+	return UpdateYAMLFile(cfg, currentVersion, newVersion)
+}

internal/files/yaml.go

@@ -88,33 +88,73 @@ func UpdateYAMLFile(cfg VersionFileConfig, currentVersion, newVersion string) er
 	return nil
 }
 
+// replacementRule defines a pattern-replacement pair for surgical YAML value replacement.
+// Each rule targets a specific quote style or value format in YAML files.
+type replacementRule struct {
+	// name describes what this rule handles (for debugging/documentation)
+	name string
+	// pattern returns the regex pattern to match for the given old value
+	pattern func(oldValue string) string
+	// replacement returns the replacement string for the given new value
+	replacement func(newValue string) string
+}
+
+// replacementRules defines all the rules for surgical YAML value replacement.
+// Rules are tried in order; the first matching rule is applied.
+var replacementRules = []replacementRule{
+	{
+		// Handles double-quoted values: key: "value"
+		name: "double-quoted",
+		pattern: func(oldValue string) string {
+			return fmt.Sprintf(`"(%s)"`, regexp.QuoteMeta(oldValue))
+		},
+		replacement: func(newValue string) string {
+			return fmt.Sprintf(`"%s"`, newValue)
+		},
+	},
+	{
+		// Handles single-quoted values: key: 'value'
+		name: "single-quoted",
+		pattern: func(oldValue string) string {
+			return fmt.Sprintf(`'(%s)'`, regexp.QuoteMeta(oldValue))
+		},
+		replacement: func(newValue string) string {
+			return fmt.Sprintf(`'%s'`, newValue)
+		},
+	},
+	{
+		// Handles unquoted values at end of line: key: value\n
+		name: "unquoted-eol",
+		pattern: func(oldValue string) string {
+			return fmt.Sprintf(`: (%s)(\s*)$`, regexp.QuoteMeta(oldValue))
+		},
+		replacement: func(newValue string) string {
+			return fmt.Sprintf(`: %s$2`, newValue)
+		},
+	},
+	{
+		// Handles unquoted values followed by inline comment: key: value # comment
+		name: "unquoted-with-comment",
+		pattern: func(oldValue string) string {
+			return fmt.Sprintf(`: (%s)(\s*#)`, regexp.QuoteMeta(oldValue))
+		},
+		replacement: func(newValue string) string {
+			return fmt.Sprintf(`: %s$2`, newValue)
+		},
+	},
+}
+
 // surgicalReplace performs a targeted replacement of a YAML value while preserving
 // the original formatting (quotes, whitespace, etc.)
 func surgicalReplace(data []byte, oldValue, newValue string) ([]byte, error) {
 	content := string(data)
 
-	// Try different quote styles that might wrap the value
-	patterns := []string{
-		// Double quoted: key: "value"
-		fmt.Sprintf(`"(%s)"`, regexp.QuoteMeta(oldValue)),
-		// Single quoted: key: 'value'
-		fmt.Sprintf(`'(%s)'`, regexp.QuoteMeta(oldValue)),
-		// Unquoted after colon: key: value
-		fmt.Sprintf(`: (%s)(\s*)$`, regexp.QuoteMeta(oldValue)),
-		fmt.Sprintf(`: (%s)(\s*#)`, regexp.QuoteMeta(oldValue)),
-	}
-
-	replacements := []string{
-		fmt.Sprintf(`"%s"`, newValue),
-		fmt.Sprintf(`'%s'`, newValue),
-		fmt.Sprintf(`: %s$2`, newValue),
-		fmt.Sprintf(`: %s$2`, newValue),
-	}
-
-	for i, pattern := range patterns {
+	// Try each replacement rule in order; use the first one that matches
+	for _, rule := range replacementRules {
+		pattern := rule.pattern(oldValue)
 		re := regexp.MustCompile(`(?m)` + pattern)
 		if re.MatchString(content) {
-			result := re.ReplaceAllString(content, replacements[i])
+			result := re.ReplaceAllString(content, rule.replacement(newValue))
 			return []byte(result), nil
 		}
 	}

internal/github/client.go

@@ -18,18 +18,49 @@ package github
 import (
 	"context"
 	"fmt"
+	"os"
 
 	"github.com/google/go-github/v60/github"
 	"golang.org/x/oauth2"
 )
 
-// Client wraps the GitHub API client.
+// PRCreator defines the interface for creating pull requests.
+type PRCreator interface {
+	// CreateReleasePR creates a new branch with the modified files and opens a PR.
+	CreateReleasePR(ctx context.Context, req PRRequest) (*PRResult, error)
+}
+
+// Client wraps the GitHub API client and implements PRCreator.
 type Client struct {
-	client *github.Client
+	client     *github.Client
+	fileReader FileReader
+}
+
+// Ensure Client implements PRCreator at compile time.
+var _ PRCreator = (*Client)(nil)
+
+// osFileReader is the default FileReader implementation that uses os.ReadFile.
+type osFileReader struct{}
+
+// ReadFile reads the contents of a file using the standard library os.ReadFile.
+func (*osFileReader) ReadFile(path string) ([]byte, error) {
+	return os.ReadFile(path)
+}
+
+// ClientOption is a functional option for configuring the Client.
+type ClientOption func(*Client)
+
+// WithFileReader sets a custom FileReader implementation for the Client.
+// This is useful for testing or when file reading needs to be customized.
+func WithFileReader(fr FileReader) ClientOption {
+	return func(c *Client) {
+		c.fileReader = fr
+	}
 }
 
 // NewClient creates a new GitHub client with the provided token.
-func NewClient(ctx context.Context, token string) (*Client, error) {
+// Optional ClientOption functions can be provided to customize the client behavior.
+func NewClient(ctx context.Context, token string, opts ...ClientOption) (*Client, error) {
 	if token == "" {
 		return nil, fmt.Errorf("token is required")
 	}
@@ -39,9 +70,16 @@ func NewClient(ctx context.Context, token string) (*Client, error) {
 	)
 	tc := oauth2.NewClient(ctx, ts)
 
-	return &Client{
-		client: github.NewClient(tc),
-	}, nil
+	c := &Client{
+		client:     github.NewClient(tc),
+		fileReader: &osFileReader{},
+	}
+
+	for _, opt := range opts {
+		opt(c)
+	}
+
+	return c, nil
 }
 
 // PRRequest contains the parameters for creating a pull request.

internal/github/client_test.go

@@ -140,3 +140,52 @@ func TestPRRequest_Validate(t *testing.T) {
 		})
 	}
 }
+
+// mockFileReader is a simple mock implementation for testing FileReader injection.
+type mockFileReader struct {
+	called bool
+}
+
+func (m *mockFileReader) ReadFile(_ string) ([]byte, error) {
+	m.called = true
+	return []byte("mock content"), nil
+}
+
+func TestWithFileReader(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name       string
+		fileReader FileReader
+	}{
+		{
+			name:       "custom FileReader is injected",
+			fileReader: &mockFileReader{},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			client, err := NewClient(context.Background(), "test-token", WithFileReader(tt.fileReader))
+			if err != nil {
+				t.Fatalf("NewClient() unexpected error = %v", err)
+			}
+			if client.fileReader != tt.fileReader {
+				t.Error("WithFileReader() did not inject the custom FileReader")
+			}
+		})
+	}
+}
+
+func TestClient_ImplementsPRCreator(t *testing.T) {
+	t.Parallel()
+
+	client, err := NewClient(context.Background(), "test-token")
+	if err != nil {
+		t.Fatalf("NewClient() unexpected error = %v", err)
+	}
+
+	// Runtime assertion that Client implements PRCreator interface.
+	var _ PRCreator = client
+}

internal/github/interfaces.go

@@ -0,0 +1,24 @@
+// Copyright 2025 Stacklok, 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 github
+
+// FileReader defines the interface for reading file contents.
+// This abstraction allows for dependency injection and makes the client
+// testable by enabling mock file systems.
+type FileReader interface {
+	// ReadFile reads the contents of a file at the given path.
+	// It returns the file contents as a byte slice, or an error if the read fails.
+	ReadFile(path string) ([]byte, error)
+}

internal/github/pr.go

@@ -17,7 +17,6 @@ package github
 import (
 	"context"
 	"fmt"
-	"os"
 	"path/filepath"
 
 	"github.com/google/go-github/v60/github"
@@ -46,8 +45,6 @@ func (c *Client) CreateReleasePR(ctx context.Context, req PRRequest) (*PRResult,
 		return nil, fmt.Errorf("creating branch: %w", err)
 	}
 
-	fmt.Printf("Created branch: %s\n", req.HeadBranch)
-
 	// Commit the files to the new branch
 	for _, filePath := range req.Files {
 		if err := c.commitFile(ctx, req.Owner, req.Repo, req.HeadBranch, filePath); err != nil {
@@ -66,12 +63,8 @@ func (c *Client) CreateReleasePR(ctx context.Context, req PRRequest) (*PRResult,
 		return nil, fmt.Errorf("creating pull request: %w", err)
 	}
 
-	// Add release label
-	_, _, err = c.client.Issues.AddLabelsToIssue(ctx, req.Owner, req.Repo, pr.GetNumber(), []string{"release"})
-	if err != nil {
-		// Non-fatal - label might not exist
-		fmt.Printf("Warning: could not add 'release' label: %v\n", err)
-	}
+	// Add release label (non-fatal if it fails, label might not exist)
+	_, _, _ = c.client.Issues.AddLabelsToIssue(ctx, req.Owner, req.Repo, pr.GetNumber(), []string{"release"})
 
 	return &PRResult{
 		Number: pr.GetNumber(),
@@ -81,8 +74,8 @@ func (c *Client) CreateReleasePR(ctx context.Context, req PRRequest) (*PRResult,
 
 // commitFile commits a single file to a branch.
 func (c *Client) commitFile(ctx context.Context, owner, repo, branch, filePath string) error {
-	// Read file content
-	content, err := os.ReadFile(filePath)
+	// Read file content using the fileReader interface
+	content, err := c.fileReader.ReadFile(filePath)
 	if err != nil {
 		return fmt.Errorf("reading file: %w", err)
 	}

internal/version/version.go

@@ -119,17 +119,34 @@ func cmpInt(a, b int) int {
 	return 0
 }
 
-// IsGreater returns true if version a is greater than version b.
-func IsGreater(a, b string) bool {
+// CompareVersions compares two version strings and returns their relative ordering.
+// It returns -1 if a < b, 0 if a == b, and 1 if a > b.
+// If either version string cannot be parsed, an error is returned with context
+// indicating which version failed to parse.
+func CompareVersions(a, b string) (int, error) {
 	va, err := Parse(a)
 	if err != nil {
-		return false
+		return 0, fmt.Errorf("parsing version a %q: %w", a, err)
 	}
 
 	vb, err := Parse(b)
 	if err != nil {
-		return false
+		return 0, fmt.Errorf("parsing version b %q: %w", b, err)
 	}
 
-	return va.Compare(vb) > 0
+	return va.Compare(vb), nil
+}
+
+// IsGreaterE returns true if version a is greater than version b, along with
+// any error that occurred during parsing. This is the preferred function for
+// new code as it allows callers to distinguish between "version A is not greater"
+// and "invalid version string".
+//
+// If an error is returned, the boolean result should be ignored.
+func IsGreaterE(a, b string) (bool, error) {
+	cmp, err := CompareVersions(a, b)
+	if err != nil {
+		return false, err
+	}
+	return cmp > 0, nil
 }