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

Added doc, benchmark and deps/X node core PR labels #32

Merged
merged 3 commits into from
Apr 27, 2016

Conversation

phillipj
Copy link
Member

@phillipj phillipj commented Apr 22, 2016

This improves the current node core auto PR labelling functionality by supporting these extra labels:

  • doc for exclusive doc/* changes
  • benchmark for exclusive benchmark/* changes
  • v8, libuv, openssl etc when having changes in deps/X/*

Also ignores changes to src/node_version.h to avoid labelling that as c++ as mentioned in #31 (comment).

@jbergstroem @mscdex @Fishrock123 does these labels sound reasonable to you?

Refs #31, nodejs/node#6247

@phillipj phillipj mentioned this pull request Apr 26, 2016
4 tasks
@phillipj
Copy link
Member Author

I'm merging this as no objections has been raised.

@phillipj phillipj merged commit d719ee1 into master Apr 27, 2016
@phillipj phillipj deleted the exclusive-labels branch April 27, 2016 18:57
@williamkapke
Copy link
Contributor

post LGTM! ;)

@phillipj
Copy link
Member Author

@williamkapke thanks! 😄

// order of entries in this map *does* matter for the resolved labels
const subSystemLabelsMap = {
// don't want to label it a c++ update when we're "only" bumping the Node.js version
'c++': /^src\/(?!node_version\.h)/,

This comment was marked as off-topic.

@Fishrock123
Copy link
Contributor

belated LGTM. I'll try to find time to tackle some of the more complex ones soon

@Fishrock123
Copy link
Contributor

nodejs/node#6432 Seems to have been labeled as test when it doesn't only contain tests. Investigating.

@Fishrock123
Copy link
Contributor

Hmm, seems like this doesn't only label test, doc, benchmark if it only contains those files, but rather if it is only one of those labels. I'll try to fix tomorrow.

@phillipj
Copy link
Member Author

Hmm, seems like this doesn't only label test, doc, benchmark if it only contains those files, but rather if it is only one of those labels. I'll try to fix tomorrow.

Woops, cause the old isOnly() got removed and we're obviously missing a test for that behavior.

@Fishrock123 goodie! I'll be ready to deploy as soon as you've got a fix ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants