-
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
Conversation
…proval + rely on github teams for directors and comm
lib/publisher.js
Outdated
processRules: metadata.get('processRules'), | ||
implementationReport: metadata.get('implementationReport'), | ||
implementationFeedbackDue: new Moment(metadata.get('implementationFeedbackDue', 'YYYY-MM-DD') | ||
.format('YYYY-MM-DD'), |
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 like 2018-6-1
to 2018-06-01
. By default, specberus doesn't provide the leading 0
.
}); | ||
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 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?
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.
Yes, that's a requirement from pubrules these days.
lib/transition-checker.js
Outdated
{ | ||
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 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?
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.
Yes, good idea!
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.
@deniak, 👍!
Just the two new comments inline, and these other reminders or notes for the future:
- This fixes Design checking of transition requests (specifically, CR) as GH issues #523 too.
- I forgot to mention to you that
octokat
does not support GH API v4 (GraphQL). Not an issue atm, but if they don't upgrade, at some point in the future we may need to use another package (in Echidna and elsewhere). Just something to consider. - Will you update the page How to use Echidna when it's merged?
- Why does w3c/transitions contains a copy of the documentation Organize a Technical Report Transition (2018 Process)? If GH is where we want to maintain that page, perhaps the README should link to the GH Pages one, and not to
w3.org/Guide/transitions
?
Ok, fair concern.
Yes, I will document the full process on a wiki page. I'll ping you when it's done so you can review.
https://www.w3.org/Guide/transitions actually proxies the github pages (see rewrite rules). We advertise that link rather than the github.io one because the official doc should be on w3.org. At least, that's my understanding. |
Fix #522 (workflow described in the tickets)
Fix #524
This PR adds support to publish CRs in echidna.
There's an additional optional parameters to specify if the CR contains substantive changes or not. By default, we assume it does.