-
Notifications
You must be signed in to change notification settings - Fork 18
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.
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
fix(1082): [6] add regex validation of commit branch filter. #247
fix(1082): [6] add regex validation of commit branch filter. #247
Conversation
config/regex.js
Outdated
@@ -49,6 +49,8 @@ module.exports = { | |||
// Can be ~pr, ~commit, or ~commit:branchName, or ~sd@123:component | |||
// Note: if you modify this regex, you must modify `sdJoi` definition in the `config/job.js` | |||
TRIGGER: /^~(sd@\d+:[\w-]+|pr|commit(:(.+))?)$/, | |||
// Can be ~commit or ~commit:master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~commit:branchName
is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I will fix this comment too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise LGTM
config/job.js
Outdated
name: 'commitBranch', | ||
validate(params, value, state, options) { | ||
const matched = Regex.TRIGGER.exec(value); | ||
const brFilter = matched[3]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be a magic number.
…into commit-br-regex
config/job.js
Outdated
validate(params, value, state, options) { | ||
const regexPos = 3; | ||
const matched = Regex.TRIGGER.exec(value); | ||
const brFilter = matched[regexPos]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also add comments to these parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry my variable name is not good I will fix the name regexPos to specificBranchPos.
After this correction and see the Regex.TRIGGER, maybe this code give you a sense.
Is this OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…into commit-br-regex
config/job.js
Outdated
{ | ||
name: 'commitBranch', | ||
validate(params, value, state, options) { | ||
const specificBranchPos = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's more clear this is a constant/hardcoded value if you put it in all caps like https://github.com/screwdriver-cd/executor-k8s-vm/blob/master/index.js#L14
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment, otherwise LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Context
SCM Branch-specific jobs
Objective
This PR adds regex validation of commit branch filter (e.g.
/^user-.*$/
).References