diff --git a/Makefile b/Makefile index 7fa8f0d..23b3c0d 100644 --- 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 index ec44ea4..8747228 100644 --- 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 index 9a3a010..10597ee 100644 --- 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 index 02b1061..4c1177b 100644 --- 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 index f056248..1b780eb 100644 --- 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 index 0d89876..d8794a3 100644 --- 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 index 539dc6b..e5ed577 100644 --- 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 index a7ae7d8..ee8f9e0 100644 --- 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 index 019d051..7e24d0d 100644 --- 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 index 2d2c5e1..0fe68a7 100644 --- 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)) }