Skip to content

fix branch slug #10698

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cleverhu wants to merge 1 commit into from
Closed

fix branch slug #10698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions applicationset/generators/pull_request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"time"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/validation"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/gosimple/slug"
Expand Down Expand Up @@ -73,10 +74,10 @@ func (g *PullRequestGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha
params := make([]map[string]interface{}, 0, len(pulls))

// In order to follow the DNS label standard as defined in RFC 1123,
// we need to limit the 'branch' to 50 to give room to append/suffix-ing it
// with 13 more characters. Also, there is the need to clean it as recommended
// we need to limit the 'branch' to 63.
// Also, there is the need to clean it as recommended
// here https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names
slug.MaxLength = 50
slug.MaxLength = 63

Comment on lines 76 to 81
Copy link
Copy Markdown
Member

@jannfis jannfis Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, is the comment just plainly wrong? It talks about suffixing the slug with 13 additional characters. If we increase the max. length to 63 here, it can't be suffixed anymore.

Can you please elaborate on this change?

Copy link
Copy Markdown
Contributor Author

@cleverhu cleverhu Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the code, i don't find the code for generate. So I think it is ok to change the max length of branch-slug to 63, and it is was mentioned in #10622.

// Converting underscores to dashes
slug.CustomSub = map[string]string{
Expand All @@ -90,10 +91,16 @@ func (g *PullRequestGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha
shortSHALength = len(pull.HeadSHA)
}

branchSlug := slug.Make(pull.Branch)
errors := validation.IsDNS1123Label(branchSlug)
if len(errors) > 0 {
return nil, fmt.Errorf("invalid branch slug: %v for branch %v: %v", branchSlug, pull.Branch, errors)
}

params = append(params, map[string]interface{}{
"number": strconv.Itoa(pull.Number),
"branch": pull.Branch,
"branch_slug": slug.Make(pull.Branch),
"branch_slug": branchSlug,
"head_sha": pull.HeadSHA,
"head_short_sha": pull.HeadSHA[:shortSHALength],
})
Expand Down
27 changes: 26 additions & 1 deletion applicationset/generators/pull_request_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,38 @@ func TestPullRequestGithubGenerateParams(t *testing.T) {
{
"number": "2",
"branch": "feat/areally+long_pull_request_name_to_test_argo_slugification_and_branch_name_shortening_feature",
"branch_slug": "feat-areally-long-pull-request-name-to-test-argo",
"branch_slug": "feat-areally-long-pull-request-name-to-test-argo-slugification",
"head_sha": "9b34ff5bd418e57d58891eb0aa0728043ca1e8be",
"head_short_sha": "9b34ff5b",
},
},
expectedErr: nil,
},
{
selectFunc: func(context.Context, *argoprojiov1alpha1.PullRequestGenerator, *argoprojiov1alpha1.ApplicationSet) (pullrequest.PullRequestService, error) {
return pullrequest.NewFakeService(
ctx,
[]*pullrequest.PullRequest{
&pullrequest.PullRequest{
Number: 2,
Branch: "abdominoposterior-pneumonoultramicroscopicsilicovolcanoconiosis-longest-branch-name",
HeadSHA: "09ae444a85e105b34abd0f15dbde72938580f970",
},
},
nil,
)
},
expected: []map[string]interface{}{
{
"number": "2",
"branch": "abdominoposterior-pneumonoultramicroscopicsilicovolcanoconiosis-longest-branch-name",
"branch_slug": "abdominoposterior-pneumonoultramicroscopicsilicovolcanoconiosis",
"head_sha": "09ae444a85e105b34abd0f15dbde72938580f970",
"head_short_sha": "09ae444a",
},
},
expectedErr: nil,
},
{
selectFunc: func(context.Context, *argoprojiov1alpha1.PullRequestGenerator, *argoprojiov1alpha1.ApplicationSet) (pullrequest.PullRequestService, error) {
return pullrequest.NewFakeService(
Expand Down
2 changes: 1 addition & 1 deletion docs/operator-manual/applicationset/Generators-Pull-Request.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ spec:

* `number`: The ID number of the pull request.
* `branch`: The name of the branch of the pull request head.
* `branch_slug`: The branch name will be cleaned to be conform to the DNS label standard as defined in [RFC 1123](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names), and truncated to 50 characters to give room to append/suffix-ing it with 13 more characters.
* `branch_slug`: The branch name will be cleaned to be conform to the DNS label standard as defined in [RFC 1123](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names).
* `head_sha`: This is the SHA of the head of the pull request.
* `head_short_sha`: This is the short SHA of the head of the pull request (8 characters long or the length of the head SHA if it's shorter).

Expand Down