Skip to content
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

Merged
merged 14 commits into from
Jun 29, 2018
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
1 change: 0 additions & 1 deletion .jscs.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
"disallowTrailingWhitespace": true,
"maximumLineLength": 160,
"requireBlocksOnNewline": 1,
"requireCamelCaseOrUpperCaseIdentifiers": true,
"requireCapitalizedComments": true,
"requireCapitalizedConstructors": true,
"requireCommaBeforeLineBreak": true,
Expand Down
5 changes: 4 additions & 1 deletion app.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ var processRequest = function (req, res, isTar) {
var decision = req.body ? req.body.decision : null;
var url = (!isTar && req.body) ? req.body.url : null;
var token = (!isTar && req.body) ? req.body.token : null;
var editorial = Boolean(req.body && req.body.editorial && /^true$/i.test(req.body.editorial));
var tar = (isTar) ? req.file : null;
var user = req.user ? req.user : null;
var dryRun = Boolean(req.body && req.body['dry-run'] && /^true$/i.test(req.body['dry-run']));
Expand All @@ -110,7 +111,8 @@ var processRequest = function (req, res, isTar) {
requests[id]['version'] = meta.version;
requests[id]['version-specberus'] = SpecberusWrapper.version;
requests[id]['decision'] = decision;
var jobList = ['retrieve-resources', 'metadata', 'specberus', 'third-party-checker', 'publish', 'tr-install', 'update-tr-shortlink'];
requests[id]['editorial'] = editorial;
var jobList = ['retrieve-resources', 'metadata', 'specberus', 'transition-checker', 'third-party-checker', 'publish', 'tr-install', 'update-tr-shortlink'];

if (isTar)
jobList.splice(2, 0, 'user-checker');
Expand All @@ -132,6 +134,7 @@ var processRequest = function (req, res, isTar) {
url,
tar,
token,
editorial,
user,
tempLocation,
httpLocation,
Expand Down
3 changes: 3 additions & 0 deletions config.js.example
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,6 @@ global.LDAP_URL = "ldaps://localhost:636";
global.LDAP_SEARCH_BASE = 'ou=user,dc=example,dc=org';
// LDAP_BIND_DN must contain the placeholder {{username}}
global.LDAP_BIND_DN = 'uid={{username}},ou=user,dc=example,dc=org';
global.GH_TOKEN = '123foobar';
global.GH_DIRECTOR_TEAM_ID = '2797096';
global.GH_COMM_TEAM_ID = '2794457';
42 changes: 42 additions & 0 deletions lib/orchestrator.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ var SpecberusWrapper = require('./specberus-wrapper');
var ThirdPartyChecker = require('./third-party-resources-checker');
var TokenChecker = require('./token-checker');
var UserChecker = require('./user-checker');
var TransitionChecker = require('./transition-checker');

// Configuration file
require('../config.js');
Expand Down Expand Up @@ -52,13 +53,15 @@ function updateTrShortlink(uri) {
var Orchestrator = function (url,
tar,
token,
isEditorial,
user,
tempLoc,
httpLoc,
resultLoc) {
this.url = url;
this.tar = tar;
this.token = token;
this.isEditorial = isEditorial;
this.user = user;
this.tempLocation = tempLoc;
this.httpLocation = httpLoc;
Expand Down Expand Up @@ -156,6 +159,9 @@ Orchestrator.prototype.runStep = function (step) {
});
}

if (obj.has('cfe'))
state = state.addToMetadata('cfe', obj.get('cfe'));

return state;
})
.catch(function (err) {
Expand Down Expand Up @@ -325,6 +331,35 @@ Orchestrator.prototype.runSpecberus = function (httpLocation,
});
};

Orchestrator.prototype.runTransitionChecker = function (profile, latestVersion, previousVersion, isEditorial, isUpdate) {
return new Map({
name: 'transition-checker',
promise: TransitionChecker.check(profile, latestVersion, previousVersion, isEditorial, isUpdate)
.then(function (report) {
if (report.errors.isEmpty()) {
return new Map({
status: 'ok',
history: 'The document passed the transition checker.',
cfe: report.requiresCfE
});
}
else {
return new Map({
status: 'failure',
errors: report.errors,
history: 'The document failed the transition checker.'
});
}
}).catch(function (error) {
return new Map({
status: 'error',
errors: List.of(error.toString()),
history: 'An error occurred while running the transition checker.'
});
})
});
};

Orchestrator.prototype.runTokenChecker = function (latestVersion, url, token) {
return new Map({
name: 'token-checker',
Expand Down Expand Up @@ -475,6 +510,13 @@ Orchestrator.prototype.next = function (state) {
state.get('metadata').get('profile'),
state.get('metadata').get('rectrack'));
}
else if (state.hasJobStarted('transition-checker')) {
step = this.runTransitionChecker(state.get('metadata').get('profile'),
state.get('metadata').get('latestVersion'),
state.get('metadata').get('previousVersion'),
this.isEditorial,
state.get('metadata').get('updated'));
}
else if (state.hasJobStarted('token-checker')) {
step = this.runTokenChecker(
state.get('metadata').get('latestVersion'),
Expand Down
7 changes: 5 additions & 2 deletions lib/publisher.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ var Publisher = function (service) {
*/
Publisher.prototype.publish = function (metadata) {
if (!(metadata instanceof Map)) throw new TypeError('metadata must be a Map');

return this.service.post({
specversion: {
status: metadata.get('profile'),
Expand All @@ -46,7 +45,11 @@ Publisher.prototype.publish = function (metadata) {
rectrack: metadata.get('rectrack'),
informative: metadata.get('informative'),
editorDraft: metadata.get('editorDraft'),
processRules: metadata.get('processRules')
processRules: metadata.get('processRules'),
implementationReport: metadata.get('implementationReport'),
implementationFeedbackDue: new Moment(metadata.get('implementationFeedbackDue'), 'YYYY-MM-DD')
.format('YYYY-MM-DD'),
Copy link
Member

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?

Copy link
Member Author

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 like 2018-6-1 to 2018-06-01. By default, specberus doesn't provide the leading 0.

cfe: metadata.get('cfe')
}
}).then(function (out) {
var response = out.response;
Expand Down
5 changes: 4 additions & 1 deletion lib/specberus-wrapper.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,11 @@ SpecberusWrapper.validate = function (url, profile, isRecTrack) {
else if (profile === 'WG-NOTE') {
specberusProfile = require('specberus/lib/profiles/TR/WG-NOTE-Echidna');
}
else if (profile === 'CR') {
specberusProfile = require('specberus/lib/profiles/TR/CR-Echidna');
}
else {
return reject(new Error('Only WD and Notes are allowed!'));
return reject(new Error('Only WD, CR and Notes are allowed!'));
}

sink.on('end-all', function () {
Expand Down
86 changes: 86 additions & 0 deletions lib/transition-checker.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
'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];
Copy link
Member

@tripu tripu Jun 26, 2018

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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',
sort: 'updated',
per_page: 100 // TODO: get all the issues, not just the first 100
Copy link
Member

Choose a reason for hiding this comment

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

Until we retrieve them all: perhaps add a param sort: 'updated' to make sure we always get the 100 that were most recently updated?

Copy link
Member Author

Choose a reason for hiding this comment

The 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;
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
"moment": "2.22.2",
"multer": "1.3.0",
"node-uuid": "1.4.8",
"octokat": "0.10.0",
"passport": "0.4.0",
"passport-http": "0.3",
"promise": "8.0.1",
Expand Down