From 8f491672e88a769858ad9f51a644500ab0ea858d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A1nchez=20Navarro?= Date: Thu, 6 Jun 2024 11:21:43 +0200 Subject: [PATCH 1/9] chore: initial commit From 8aebb0a66e6803c3e1cc92bdaf3b2f7ada11979d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A1nchez=20Navarro?= Date: Thu, 6 Jun 2024 12:40:10 +0200 Subject: [PATCH 2/9] feat: new branch configuration param to control branch length --- internal/branches/branches.go | 8 +++- internal/branches/branches_test.go | 46 +++++++++++++++++-- internal/branches/interactive.go | 13 ++++-- internal/config/branches.go | 3 +- internal/config/default-config.yml | 4 ++ .../config/testdata/test-configuration.yml | 1 + 6 files changed, 66 insertions(+), 9 deletions(-) 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..50c85fe 100644 --- a/internal/branches/branches_test.go +++ b/internal/branches/branches_test.go @@ -45,6 +45,7 @@ func TestFormatBranchName(t *testing.T) { issueId string issueContext string branchPrefixOverride map[issue_types.IssueType]string + maxLength int } tests := []struct { name string @@ -53,7 +54,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 +72,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 +95,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 +107,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 +141,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/default-config.yml b/internal/config/default-config.yml index 539dc6b..a7ce4ea 100644 --- a/internal/config/default-config.yml +++ b/internal/config/default-config.yml @@ -77,3 +77,7 @@ branches: # bugfix: myfix # Example: map `feature` type to a branch prefix `feat/xxxx`: # feature: feat + # Bran max length configuration + # Useful when you want to crop the branch name to a specific length. + # By default it will use 63 due to compatibilities with Kubernetes. + 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 From 42800b401e6c468edc1e9128235fcafc85183843 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A1nchez=20Navarro?= Date: Thu, 6 Jun 2024 13:02:43 +0200 Subject: [PATCH 3/9] feat: custom error message for gte validation --- pkg/validator/error_messages.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/pkg/validator/error_messages.go b/pkg/validator/error_messages.go index 2d2c5e1..00a6bfa 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 validationErroMessagesWithParam = 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 = validationErroMessagesWithParam[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)) } From b7445e0d167c14e911e944ead828cd26ae574250 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A1nchez=20Navarro?= Date: Thu, 6 Jun 2024 14:25:16 +0200 Subject: [PATCH 4/9] test: add max length validator test --- internal/config/configuration_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) 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) + }) + } From 9447fee7c51384cc80a611b91b8306e0209747b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A1nchez=20Navarro?= Date: Thu, 6 Jun 2024 14:34:28 +0200 Subject: [PATCH 5/9] chore: update coverage command in Makefile to use coverpkg flag --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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: From 14f7012a2818d175e1dd935b75baca6b5ddbe442 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A1nchez=20Navarro?= Date: Thu, 6 Jun 2024 16:59:50 +0200 Subject: [PATCH 6/9] test: add test cases for get branch name function We still have to add more tests here --- internal/branches/branches_test.go | 98 +++++++++++++++++++++++++++++ internal/fakes/domain/fake_issue.go | 20 ++++-- 2 files changed, 114 insertions(+), 4 deletions(-) diff --git a/internal/branches/branches_test.go b/internal/branches/branches_test.go index 50c85fe..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 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 } From 2e463bdeed1c0e88ea1551399e69b4629ad47af1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A1nchez?= Date: Thu, 6 Jun 2024 17:42:35 +0200 Subject: [PATCH 7/9] chore: Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ismael González Valverde <80819220+ismaelgonval@users.noreply.github.com> --- internal/config/default-config.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/config/default-config.yml b/internal/config/default-config.yml index a7ce4ea..97302cf 100644 --- a/internal/config/default-config.yml +++ b/internal/config/default-config.yml @@ -77,7 +77,7 @@ branches: # bugfix: myfix # Example: map `feature` type to a branch prefix `feat/xxxx`: # feature: feat - # Bran max length configuration + # 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 due to compatibilities with Kubernetes. + # By default it will use 63 for Kubernetes resources compatibility. max_length: 63 From b1563c1c129f28a6fb4433f613560d07498d9209 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A1nchez=20Navarro?= Date: Thu, 6 Jun 2024 17:43:15 +0200 Subject: [PATCH 8/9] chore: fix typo in variable name --- pkg/validator/error_messages.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/validator/error_messages.go b/pkg/validator/error_messages.go index 00a6bfa..0fe68a7 100644 --- a/pkg/validator/error_messages.go +++ b/pkg/validator/error_messages.go @@ -15,7 +15,7 @@ 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 validationErroMessagesWithParam = map[string]string{ +var validationErrorMessagesWithParam = map[string]string{ "gte": "Must be greater than or equal to %s", } @@ -26,7 +26,7 @@ func getPrettyErrors(validationErrors govalidator.ValidationErrors) string { errKey := fieldErr.Namespace() errMsg, ok := validationErrorMessages[fieldErr.Tag()] if !ok { - errMsg, ok = validationErroMessagesWithParam[fieldErr.Tag()] + errMsg, ok = validationErrorMessagesWithParam[fieldErr.Tag()] if !ok { errMsg = fmt.Sprintf(fallbackErrMessage, fieldErr.Field(), fieldErr.Tag()) } else { From 5e93512558779ecfb60441d5d5a61d1a76710f85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A1nchez=20Navarro?= Date: Thu, 6 Jun 2024 17:52:58 +0200 Subject: [PATCH 9/9] docs: Add comment to explain how to disable max branch name limit --- internal/config/default-config.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/config/default-config.yml b/internal/config/default-config.yml index 97302cf..e5ed577 100644 --- a/internal/config/default-config.yml +++ b/internal/config/default-config.yml @@ -80,4 +80,5 @@ branches: # 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