Branch Name Length Control Configurable (#93)

* chore: initial commit * feat: new branch configuration param to control branch length * feat: custom error message for gte validation * test: add max length validator test * chore: update coverage command in Makefile to use coverpkg flag * test: add test cases for get branch name function We still have to add more tests here * chore: Apply suggestions from code review Co-authored-by: Ismael González Valverde <[email protected]> * chore: fix typo in variable name * docs: Add comment to explain how to disable max branch name limit --------- Co-authored-by: Ismael González Valverde <[email protected]>
InditexTech · Jun 6, 2024 · 2fa2c6e · 2fa2c6e
1 parent 6d99e6e
commit 2fa2c6e
Show file tree

Hide file tree

Showing 10 changed files with 201 additions and 15 deletions.
diff --git a/Makefile b/Makefile
@@ -47,7 +47,7 @@ coverage:
 	$(eval TEST_COVERAGE_PROFILE_OUTPUT_DIRNAME=$(shell dirname $(TEST_COVERAGE_PROFILE_OUTPUT)))
 	$(eval TEST_REPORT_OUTPUT_DIRNAME=$(shell dirname $(TEST_REPORT_OUTPUT)))
 	mkdir -p $(TEST_COVERAGE_PROFILE_OUTPUT_DIRNAME) $(TEST_REPORT_OUTPUT_DIRNAME)
-	$(GO) test ./... -coverprofile=$(TEST_COVERAGE_PROFILE_OUTPUT) -json > $(TEST_REPORT_OUTPUT)
+	$(GO) test ./... -coverpkg=./... -coverprofile=$(TEST_COVERAGE_PROFILE_OUTPUT) -json > $(TEST_REPORT_OUTPUT)
 
 .PHONY: generate-mocks
 generate-mocks:

diff --git a/internal/branches/branches.go b/internal/branches/branches.go
@@ -114,7 +114,13 @@ func (b BranchProvider) formatBranchName(repoNameWithOwner string, branchType st
 		branchName = fmt.Sprintf("%s-%s", branchName, issueContext)
 	}
 
-	return strings.TrimSuffix(branchName[:min(63-len([]rune(repoNameWithOwner)), len([]rune(branchName)))], "-")
+	maxBranchNameLength := b.cfg.MaxLength - len([]rune(repoNameWithOwner))
+	if maxBranchNameLength > 0 {
+		branchNameLength := len([]rune(branchName))
+		branchName = branchName[:min(maxBranchNameLength, branchNameLength)]
+	}
+
+	return strings.TrimSuffix(branchName, "-")
 }
 
 // min returns the smallest of x or y.

diff --git a/internal/branches/branches_test.go b/internal/branches/branches_test.go
@@ -4,12 +4,110 @@ import (
 	"testing"
 
 	"github.com/InditexTech/gh-sherpa/internal/config"
+	"github.com/InditexTech/gh-sherpa/internal/domain"
 	"github.com/InditexTech/gh-sherpa/internal/domain/issue_types"
+	domainFakes "github.com/InditexTech/gh-sherpa/internal/fakes/domain"
 	domainMocks "github.com/InditexTech/gh-sherpa/internal/mocks/domain"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
+	"github.com/stretchr/testify/suite"
 )
 
+type BranchTestSuite struct {
+	suite.Suite
+	b                       *BranchProvider
+	userInteractionProvider *domainMocks.MockUserInteractionProvider
+	fakeIssue               *domainFakes.FakeIssue
+	defaultRepository       *domain.Repository
+}
+
+func TestBranchTestSuite(t *testing.T) {
+	suite.Run(t, new(BranchTestSuite))
+}
+
+func (s *BranchTestSuite) SetupSubTest() {
+	s.userInteractionProvider = &domainMocks.MockUserInteractionProvider{}
+
+	s.b = &BranchProvider{
+		cfg: Configuration{
+			Branches: config.Branches{
+				Prefixes: map[issue_types.IssueType]string{
+					issue_types.Bug: "bugfix",
+				},
+				MaxLength: 0,
+			},
+		},
+		UserInteraction: s.userInteractionProvider,
+	}
+
+	s.defaultRepository = &domain.Repository{
+		Name:             "test-name",
+		Owner:            "test-owner",
+		NameWithOwner:    "test-owner/test-name",
+		DefaultBranchRef: "main",
+	}
+
+	s.fakeIssue = domainFakes.NewFakeIssue("1", issue_types.Bug, domain.IssueTrackerTypeGithub)
+}
+
+func (s *BranchTestSuite) TestGetBranchName() {
+
+	s.Run("should return expected branch name", func() {
+		expectedBrachName := "bugfix/GH-1-fake-title"
+
+		branchName, err := s.b.GetBranchName(s.fakeIssue, *s.defaultRepository)
+
+		s.NoError(err)
+		s.Equal(expectedBrachName, branchName)
+	})
+
+	s.Run("should return cropped branch", func() {
+		expectedBrachName := "bugfix/GH-1-my-title-is-too-long-and-it-sho"
+
+		s.fakeIssue.SetTitle("my title is too long and it should not matter")
+
+		s.b.cfg.Branches.MaxLength = 63
+		branchName, err := s.b.GetBranchName(s.fakeIssue, *s.defaultRepository)
+
+		s.NoError(err)
+		s.Equal(expectedBrachName, branchName)
+	})
+
+	s.Run("should return expected branch name when interactive", func() {
+		expectedBrachName := "bugfix/GH-1-fake-title-from-interactive"
+
+		s.userInteractionProvider.EXPECT().SelectOrInputPrompt(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()
+		s.userInteractionProvider.EXPECT().SelectOrInput(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Run(func(name string, validValues []string, variable *string, required bool) {
+			*variable = "fake title from interactive"
+		}).Return(nil).Once()
+
+		s.b.cfg.IsInteractive = true
+
+		branchName, err := s.b.GetBranchName(s.fakeIssue, *s.defaultRepository)
+
+		s.NoError(err)
+		s.Equal(expectedBrachName, branchName)
+	})
+
+	s.Run("should return cropped branch name when interactive", func() {
+		expectedBrachName := "bugfix/GH-1-this-is-a-very-long-fake-title"
+
+		s.userInteractionProvider.EXPECT().SelectOrInputPrompt(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()
+		s.userInteractionProvider.EXPECT().SelectOrInput(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Run(func(name string, validValues []string, variable *string, required bool) {
+			*variable = "this is a very long fake title from interactive"
+		}).Return(nil).Once()
+
+		s.b.cfg.IsInteractive = true
+		s.b.cfg.Branches.MaxLength = 63
+
+		branchName, err := s.b.GetBranchName(s.fakeIssue, *s.defaultRepository)
+
+		s.NoError(err)
+		s.Equal(expectedBrachName, branchName)
+	})
+}
+
 func TestParseIssueContext(t *testing.T) {
 	tests := []struct {
 		name  string
@@ -45,6 +143,7 @@ func TestFormatBranchName(t *testing.T) {
 		issueId              string
 		issueContext         string
 		branchPrefixOverride map[issue_types.IssueType]string
+		maxLength            int
 	}
 	tests := []struct {
 		name string
@@ -53,7 +152,14 @@ func TestFormatBranchName(t *testing.T) {
 	}{
 		{
 			name: "Does format branch name",
-			args: args{repository: repositoryName, branchType: "feature", issueId: "GH-1", issueContext: "my-title"},
+			args: args{
+				repository:   repositoryName,
+				branchType:   "feature",
+				issueId:      "GH-1",
+				issueContext: "my-title",
+				maxLength:    63,
+			},
+
 			want: "feature/GH-1-my-title",
 		},
 		{
@@ -64,12 +170,19 @@ func TestFormatBranchName(t *testing.T) {
 				issueId:              "GH-1",
 				issueContext:         "my-title",
 				branchPrefixOverride: map[issue_types.IssueType]string{issue_types.Feature: "feat"},
+				maxLength:            63,
 			},
 			want: "feat/GH-1-my-title",
 		},
 		{
 			name: "Does format long branch name",
-			args: args{repository: repositoryName, branchType: "feature", issueId: "GH-1", issueContext: "my-title-is-too-long-and-it-should-be-truncated"},
+			args: args{
+				repository:   repositoryName,
+				branchType:   "feature",
+				issueId:      "GH-1",
+				issueContext: "my-title-is-too-long-and-it-should-be-truncated",
+				maxLength:    63,
+			},
 			want: "feature/GH-1-my-title-is-too-long-and-it-s",
 		},
 		{
@@ -80,6 +193,7 @@ func TestFormatBranchName(t *testing.T) {
 				issueId:              "GH-1",
 				issueContext:         "my-title-is-too-long-and-it-should-be-truncated",
 				branchPrefixOverride: map[issue_types.IssueType]string{issue_types.Feature: "feat"},
+				maxLength:            63,
 			},
 			want: "feat/GH-1-my-title-is-too-long-and-it-shou",
 		},
@@ -91,9 +205,32 @@ func TestFormatBranchName(t *testing.T) {
 				issueId:              "GH-1",
 				issueContext:         "refactor-issue",
 				branchPrefixOverride: map[issue_types.IssueType]string{issue_types.Refactoring: ""},
+				maxLength:            63,
 			},
 			want: "refactoring/GH-1-refactor-issue",
 		},
+		{
+			name: "Does not crop the title if the maxLength is 0",
+			args: args{
+				repository:   repositoryName,
+				branchType:   "feature",
+				issueId:      "GH-1",
+				issueContext: "this-is-a-very-long-title-that-will-not-be-cropped",
+				maxLength:    0,
+			},
+			want: "feature/GH-1-this-is-a-very-long-title-that-will-not-be-cropped",
+		},
+		{
+			name: "Does not crop the title if the maxLength is negative",
+			args: args{
+				repository:   repositoryName,
+				branchType:   "feature",
+				issueId:      "GH-1",
+				issueContext: "this-is-a-very-long-title-that-will-not-be-cropped",
+				maxLength:    -1,
+			},
+			want: "feature/GH-1-this-is-a-very-long-title-that-will-not-be-cropped",
+		},
 	}
 
 	for _, tt := range tests {
@@ -102,7 +239,8 @@ func TestFormatBranchName(t *testing.T) {
 			b := BranchProvider{
 				cfg: Configuration{
 					Branches: config.Branches{
-						Prefixes: tt.args.branchPrefixOverride,
+						Prefixes:  tt.args.branchPrefixOverride,
+						MaxLength: tt.args.maxLength,
 					},
 				},
 			}

diff --git a/internal/branches/interactive.go b/internal/branches/interactive.go
@@ -27,8 +27,13 @@ func (b BranchProvider) GetBranchName(issue domain.Issue, repo domain.Repository
 			return "", err
 		}
 
-		maxContextLen := calcIssueContextMaxLen(repo.NameWithOwner, branchType, formattedID)
-		promptIssueContext := fmt.Sprintf("additional description (optional). Truncate to %d chars", maxContextLen)
+		truncatePrompt := ""
+		maxContextLen := b.calcIssueContextMaxLen(repo.NameWithOwner, branchType, formattedID)
+		if maxContextLen > 0 {
+			truncatePrompt = fmt.Sprintf(" Truncate to %d chars", maxContextLen)
+		}
+
+		promptIssueContext := "additional description (optional)." + truncatePrompt
 		err = b.UserInteraction.SelectOrInput(promptIssueContext, []string{}, &issueSlug, false)
 		if err != nil {
 			return "", err
@@ -62,10 +67,10 @@ func (b BranchProvider) getBugFixBranchType() (branchType string) {
 	return branchType
 }
 
-func calcIssueContextMaxLen(repository string, branchType string, issueId string) (lenIssueContext int) {
+func (b BranchProvider) calcIssueContextMaxLen(repository string, branchType string, issueId string) (lenIssueContext int) {
 	preBranchName := fmt.Sprintf("%s/%s-", branchType, issueId)
 
-	if lenIssueContext = 63 - (len([]rune(repository)) + len([]rune(preBranchName))); lenIssueContext < 0 {
+	if lenIssueContext = b.cfg.MaxLength - (len([]rune(repository)) + len([]rune(preBranchName))); lenIssueContext < 0 {
 		lenIssueContext = 0
 	}
 

diff --git a/internal/config/branches.go b/internal/config/branches.go
@@ -3,7 +3,8 @@ package config
 import "github.com/InditexTech/gh-sherpa/internal/domain/issue_types"
 
 type Branches struct {
-	Prefixes BranchesPrefixes `validate:"validIssueTypeKeys"`
+	Prefixes  BranchesPrefixes `validate:"validIssueTypeKeys"`
+	MaxLength int              `mapstructure:"max_length" validate:"gte=0"`
 }
 
 type BranchesPrefixes map[issue_types.IssueType]string
diff --git a/internal/config/configuration_test.go b/internal/config/configuration_test.go
@@ -291,4 +291,13 @@ func (s *ValidateConfigTestSuite) TestConfigurationValidations() {
 		s.Error(err)
 	})
 
+	s.Run("Should return error if branches max length is negative", func() {
+		tCfg := s.getValidConfig()
+		tCfg.Branches.MaxLength = -1
+
+		err := tCfg.Validate()
+
+		s.Error(err)
+	})
+
 }
diff --git a/internal/config/default-config.yml b/internal/config/default-config.yml
@@ -77,3 +77,8 @@ branches:
     # bugfix: myfix
     # Example: map `feature` type to a branch prefix `feat/xxxx`:
     # feature: feat
+  # Branch name max number of characters
+  # Useful when you want to crop the branch name to a specific length.
+  # By default it will use 63 for Kubernetes resources compatibility.
+  # You can disable this limit of characters by setting this value to 0.
+  max_length: 63
diff --git a/internal/config/testdata/test-configuration.yml b/internal/config/testdata/test-configuration.yml
@@ -25,3 +25,4 @@ branches:
   prefixes:
     feature: "feat"
     bugfix: "fix"
+  max_length: 63
diff --git a/internal/fakes/domain/fake_issue.go b/internal/fakes/domain/fake_issue.go
@@ -9,22 +9,34 @@ import (
 
 type FakeIssue struct {
 	id               string
+	title            string
+	body             string
+	url              string
 	issueType        issue_types.IssueType
 	issueTrackerType domain.IssueTrackerType
+	typeLabel        string
 }
 
 var _ domain.Issue = (*FakeIssue)(nil)
 
+func (f *FakeIssue) SetTitle(title string) {
+	f.title = title
+}
+
 func NewFakeIssue(id string, issueType issue_types.IssueType, issueTrackerType domain.IssueTrackerType) *FakeIssue {
 	return &FakeIssue{
 		id:               id,
+		title:            "fake title",
+		body:             "fake body",
+		url:              "fake url",
 		issueType:        issueType,
 		issueTrackerType: issueTrackerType,
+		typeLabel:        fmt.Sprintf("kind/%s", issueType),
 	}
 }
 
 func (f *FakeIssue) Body() string {
-	return "fake body"
+	return f.body
 }
 
 func (f *FakeIssue) FormatID() string {
@@ -48,13 +60,13 @@ func (f *FakeIssue) Type() issue_types.IssueType {
 }
 
 func (f *FakeIssue) TypeLabel() string {
-	return fmt.Sprintf("kind/%s", f.issueType)
+	return f.typeLabel
 }
 
 func (f *FakeIssue) Title() string {
-	return "fake title"
+	return f.title
 }
 
 func (f *FakeIssue) URL() string {
-	return "fake url"
+	return f.url
 }
diff --git a/pkg/validator/error_messages.go b/pkg/validator/error_messages.go
@@ -15,6 +15,9 @@ var validationErrorMessages = map[string]string{
 	"validIssueTypeKeys": "Keys must be a valid issue type. Check the documentation for the list of valid issue types",
 	"uniqueMapValues":    "Values must be unique across all keys. Check the default values for possible collisions",
 }
+var validationErrorMessagesWithParam = map[string]string{
+	"gte": "Must be greater than or equal to %s",
+}
 
 func getPrettyErrors(validationErrors govalidator.ValidationErrors) string {
 	var buffer bytes.Buffer
@@ -23,8 +26,14 @@ func getPrettyErrors(validationErrors govalidator.ValidationErrors) string {
 		errKey := fieldErr.Namespace()
 		errMsg, ok := validationErrorMessages[fieldErr.Tag()]
 		if !ok {
-			errMsg = fmt.Sprintf(fallbackErrMessage, fieldErr.Field(), fieldErr.Tag())
+			errMsg, ok = validationErrorMessagesWithParam[fieldErr.Tag()]
+			if !ok {
+				errMsg = fmt.Sprintf(fallbackErrMessage, fieldErr.Field(), fieldErr.Tag())
+			} else {
+				errMsg = fmt.Sprintf(errMsg, fieldErr.Param())
+			}
 		}
+
 		buffer.WriteString(fmt.Sprintf("- %s: %s\n", errKey, errMsg))
 	}