Allow custom templates for messages, branches and PRs#307
Allow custom templates for messages, branches and PRs#307EyalDelarea merged 17 commits intojfrog:devfrom
Conversation
eyalbe4
left a comment
There was a problem hiding this comment.
I haven't finished reviewing all the code. I'll resume my review soon, but I'm releasingwhat I have so far.
commands/utils/utils.go
Outdated
| if len(branchName) == 0 { | ||
| return nil | ||
| } | ||
| invalidChars := regexp.MustCompile(BranchNameRegex) |
There was a problem hiding this comment.
The compile operation is expensive, and it is therefore better to do it once only.
| {"dev", "replace:colons:colons", "3.0.0", "frogbot-replace_colons_colons-89e555131b4a70a32fe9d9c44d6ff0fc"}, | ||
| } | ||
|
|
||
| gitManager := utils.GitManager{} |
There was a problem hiding this comment.
This line is redundant. Yuo can replace gitManager with utils.GitManager{} in line 212.
There was a problem hiding this comment.
It doesn't let me run it like that.
And they are all the same so we can create it just once :)
| # [Optional] Custom formats for commits, branches, and PR created by Frogbot. | ||
| customFormats: | ||
| # Please note, Frogbot will replace the following placeholders | ||
| # 1. IMPACTED_PACKAGE | ||
| # 2. FIX_VERSION | ||
| # Example branchName:"(MyLabel):Vulnerability_fix_IMPACTED_PACKAGE_to_version_FIX_VERSION" | ||
|
|
||
| commitTitle: "" | ||
|
|
||
| pullRequestTitle: "" | ||
|
|
||
| branchName: "" |
There was a problem hiding this comment.
I suggest changing the template to be -
- params:
# Git parameters
git:
# [Mandatory]
# Name of the git repository to scan
repoName: repo-name
# [Mandatory]
# List of branches to scan
branches:
- master
# [Optional] Template for the branch name generated by Frogbot when creating pull requests with fixes. The template must include ${BRANCH_NAME_HASH}, to ensure that the generated branch name is unique
# branchNameTemplate: ""
# [Optional] Template for the commit message generated by Frogbot when creating pull requests with fixes
# commitMessageTemplate: ""
# [Optional] Template for the pull request title generated by Frogbot when creating pull requests with fixes. The template can optionally include the following variables:
# ${IMPACTED_PACKAGE}
# ${FIX_VERSION}
# pullRequestTitleTemplate: ""
Let's add our default values for each variable instead of the existing empty string.
eyalbe4
left a comment
There was a problem hiding this comment.
I'm done reviewing everything.
- I suggest that you first read all of my comments before working o fixing them, beucase some of them are related to each other.
- While suggesting the changes to the
frogbot-config.ymlfile, I realized that the proper term for for the new config items is actiually not "format" but "template". Let's therefore change all the structs' name to reflect that. - We still need to update all the workflow templates with the new variables. Let's do it in a follow-up pull request.
- I'd be happy to review everything again once done.
| branches: | ||
| - master | ||
|
|
||
| # [Optional] Template for the branch name generated by Frogbot when creating pull requests with fixes.The template must include ${BRANCH_NAME_HASH}, to ensure that the generated branch name is unique |
There was a problem hiding this comment.
Maybe we should explain the placeholder in the start here?
because they can be used in every template
Suggestion:
| # [Optional] Template for the branch name generated by Frogbot when creating pull requests with fixes.The template must include ${BRANCH_NAME_HASH}, to ensure that the generated branch name is unique | |
| # [Optional] Git templates, each template can include the following variables: | |
| # ${IMPACTED_PACKAGE} | |
| # ${FIX_VERSION} | |
| # [Optional] Template for the branch name generated by Frogbot when creating pull requests with fixes.The template must include ${BRANCH_NAME_HASH}, to ensure that the generated branch name is unique | |
| branchNameTemplate: "" | |
| # [Optional] Template for the commit message generated by Frogbot when creating pull requests with fixes | |
| CommitMessageTemplate: "" | |
| # [Optional] Template for the pull request title generated by Frogbot when creating pull requests with fixes.The template can optionally include the following variables: | |
| pullRequestTitleTemplate: "" |
There was a problem hiding this comment.
Let's think about it after the release.
commands/utils/git.go
Outdated
| commitMessageTemplate string | ||
| // New branch name template | ||
| branchNameTemplate string | ||
| // New pullRequestTitleTemplate title template |
There was a problem hiding this comment.
// New pullRequestTitleTemplate title template
-->
// New pull request title template
commands/utils/utils.go
Outdated
| BranchInvalidChars = "branch name cannot contain the following chars ~, ^, :, ?, *, [, ], @, {, }" | ||
| BranchInvalidPrefix = "branch name cannot start with '-' " | ||
| BranchCharsMaxLength = 255 | ||
| BranchInvalidLength = "branch name length exceeded " + string(rune(BranchCharsMaxLength)) + " chars" | ||
| InvalidBranchTemplate = "branch template must contain " + BranchHashPlaceHolder + " placeholder " |
There was a problem hiding this comment.
The above consts can be made private,
commands/utils/utils.go
Outdated
| if len(branchName) == 0 { | ||
| return nil | ||
| } | ||
| invalidChars := regexp.MustCompile(branchNameRegex) |
There was a problem hiding this comment.
As I suggest before, MustCompile should berun only once. You can therefore have a private const that is initialized with regexp.MustCompile(branchNameRegex).
commands/utils/utils_test.go
Outdated
| return | ||
| } | ||
|
|
||
| func TestIsValidBranchName(t *testing.T) { |
There was a problem hiding this comment.
TestIsValidBranchName --> TestValidatedBranchName
(the test name should match the function name, which is validateBranchName).
commands/utils/utils.go
Outdated
| return candidateMajorVersion != currentMajorVersion | ||
| } | ||
|
|
||
| func ValidateBranchName(branchName string) error { |
There was a problem hiding this comment.
This function can be made private,
| branches: | ||
| - master | ||
|
|
||
| # [Optional] Template for the branch name generated by Frogbot when creating pull requests with fixes.The template must include ${BRANCH_NAME_HASH}, to ensure that the generated branch name is unique |
There was a problem hiding this comment.
Let's think about it after the release.
| - master | ||
|
|
||
| # [Optional] Template for the branch name generated by Frogbot when creating pull requests with fixes.The template must include ${BRANCH_NAME_HASH}, to ensure that the generated branch name is unique | ||
| branchNameTemplate: "" |
There was a problem hiding this comment.
Let's put this line as a comment (with a # prefix) and set the default value as the value.
The same goes for the two templates below.

Exmaple:
Given templates: