-
Notifications
You must be signed in to change notification settings - Fork 33
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
CR: check CR with specberus and check approvals with GH API #575
Changes from 9 commits
3b08783
4653d0e
8afb76a
b9c2221
c232daa
770fd83
dd33182
945a6a8
71e3482
fb5a7c4
89cc24a
e25caaf
bfb1d46
bd7ec5f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
'use strict'; | ||
|
||
var List = require('immutable').List; | ||
var Octokat = require('octokat'); | ||
|
||
/** | ||
* @exports lib/transition-checker | ||
*/ | ||
|
||
var TransitionChecker = {}; | ||
|
||
/** | ||
* @returns {Promise.<List.<String>>} | ||
*/ | ||
|
||
TransitionChecker.check = function (profile, latestVersion, previousVersion, isEditorial, isUpdate) { | ||
return new Promise(function (resolve, reject) { | ||
var errors = new List(); | ||
|
||
if (profile === 'WD' || profile === 'WG-NOTE' || isUpdate) { | ||
resolve({ errors: errors, requiresCfE: false }); | ||
} | ||
else if (profile === 'CR') { | ||
if (previousVersion.includes('/WD-') || (previousVersion.includes('/CR-') && !isEditorial)) { | ||
var octo = new Octokat({ | ||
token: global.GH_TOKEN | ||
}); | ||
var repo = octo.repos('w3c', 'transitions'); | ||
|
||
var shortname = latestVersion.match(new RegExp(/.*\/([^/]+)\/$/))[1]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. About this regex: are we sure that all latest version URLs end with a slash? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's a requirement from pubrules these days. |
||
var directorApproval = 'Transition approved.'; | ||
var commApproval = 'Draft transition approved.'; | ||
var directorApprovalFound = false; | ||
var commApprovalFound = false; | ||
|
||
repo.issues.fetch( | ||
{ | ||
labels: 'Awaiting publication', | ||
state: 'open', | ||
per_page: 100 // TODO: get all the issues, not just the first 100 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Until we retrieve them all: perhaps add a param There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, good idea! |
||
}) | ||
.then((content) => { | ||
for (var issue of content.items) { | ||
if (issue.title.endsWith(' ' + shortname)) { | ||
repo.issues(issue.number).comments.fetch() | ||
.then((comments) => { | ||
for (var comment of comments.items) { | ||
if (comment.body.startsWith(directorApproval)) { | ||
// Director's approval | ||
octo.teams(global.GH_DIRECTOR_TEAM_ID).members(comment.user.login).fetch((err) => { | ||
if (!err) directorApprovalFound = true; | ||
}); | ||
} | ||
else if (comment.body.startsWith(commApproval)) { | ||
// Comm's approval | ||
octo.teams(global.GH_COMM_TEAM_ID).members(comment.user.login).fetch((err) => { | ||
if (!err) commApprovalFound = true; | ||
}); | ||
} | ||
} | ||
if (!directorApprovalFound) errors.push('Director\'s approval not found.'); | ||
if (!commApprovalFound) errors.push('Communication team\'s approval not found.'); | ||
return resolve({ errors: errors, requiresCfE: true }); | ||
}); | ||
} | ||
else { | ||
// Issue not found | ||
return reject(new Error('Issue not found on the github repository w3c/transitions.')); | ||
} | ||
} | ||
}) | ||
.catch(function (error) { | ||
return reject(new Error('An error occured while looking for the transition issue: ' + error)); | ||
}); | ||
} | ||
else if (previousVersion.includes('/CR-') && isEditorial) { | ||
return resolve({ errors: errors, requiresCfE: false }); | ||
} | ||
} | ||
else | ||
reject(new Error('Only WD, CR and Notes are allowed!')); | ||
}); | ||
}; | ||
|
||
module.exports = TransitionChecker; |
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.
There's a missing
)
in this expression, and I think the first param'YYYY-MM-DD'
shouldn't be there?Apart from that, why
new Moment(metadata.get('implementationFeedbackDue').format('YYYY-MM-DD')
?
Isn't
metadata.get('implementationFeedbackDue')
already a string with the same format that you need?
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.
Oups, I thought I had pushed the missing parenthesis but I didn't. Done now.
I use
Moment
to convert strings like2018-6-1
to2018-06-01
. By default, specberus doesn't provide the leading0
.