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

Auto label node PR's #6

Merged
merged 1 commit into from
Apr 16, 2016
Merged

Auto label node PR's #6

merged 1 commit into from
Apr 16, 2016

Conversation

phillipj
Copy link
Member

@phillipj phillipj commented Apr 4, 2016

Very much a work in progress ATM, just wanted to get this out there for public scrutiny and work on it the next couple of days.

Goals

  • lists appropriate labels for any PR on /labels/#PR-NUM
  • adds labels to created PR's

UPDATE In the discussion below, we've decided to only resolve c++ and test labels at first, then add more labels in later iterations of this.

Refs #1

@Fishrock123
Copy link
Contributor

I don't mind if it over-labels a bit. We can probably do a trial of sorts by just regexing against tag names with some sort of priority, falling back to lib / src if there are more than, say, 5.

@Fishrock123
Copy link
Contributor

the refactoring bits lgtm

@phillipj
Copy link
Member Author

Pushed some improvements. Now it does some very basic analysis on the file paths changed and returns a list of labels.

Obvious things still missing:

  • doesn't handle src changes
  • changes to lib/_*.js and lib/internal has to be mapped manually. Currently changes to lib/_stream_readable.js would be resolved to a "_stream_readable" label

@Fishrock123
Copy link
Contributor

I think it is probably more reliable to just hardcode a bunch of regex's. I'm willing to figure out what those should be.

We should also try to fallback to lib / src if there is > 5 subsystem labels

@williamkapke
Copy link
Contributor

I need the github client extracted out as you did here in lib/githubClient.js. Maybe that can go in a PR of its own?

For now I'll use a copy of that file but won't commit it.

@Fishrock123
Copy link
Contributor

@phillipj How about we make this only check against regex's and only use c++ from the start, then we can expand the map.

This will also make sure the bot never uses labels that people willy-nilly add/remove.

@phillipj
Copy link
Member Author

@Fishrock123 sure, we could start off with a limited set of labels.

What about keeping the current test label functionality in here? Adding test when the changeset only contains ./test/* files..

@Fishrock123
Copy link
Contributor

@phillipj Also seems good to me, but I think it should be cleaned up a bit to be a bit more portable.. We probably need some to check isOnly and other to check is.

Another example would be a purely benchmarks change. I don't think we should add that yet, just let it be easier to build on after, if possible. :)

@phillipj phillipj force-pushed the auto-tag-core-pr branch 2 times, most recently from 60e24d2 to 6fdd91f Compare April 14, 2016 20:14

//
// Planned tests to be resolved later
//

This comment was marked as off-topic.

@Fishrock123
Copy link
Contributor

@phillipj lgtm minus a nit, although I'm not sure we should deploy this yet. We'll want to have a go-ahead from collaborators with some example in a test repo I think.

If you think we should go straight to deploy though, I'm willing to back that.

@phillipj phillipj changed the title [WIP] auto tag node PR's Auto label node PR's Apr 15, 2016
@phillipj
Copy link
Member Author

@Fishrock123 PTAL

This branch is currently deployed and have done some initial tests in https://github.com/TestOrgPleaseIgnore/node/pulls

user: options.owner,
repo: options.repo,
number: options.prId,
labels: labels

This comment was marked as off-topic.

Started out with two labels in this iteration: c++ and test.
@phillipj
Copy link
Member Author

@Fishrock123 @williamkapke Thanks for reviewing! Just pushed an update which merges the resolved labels with the PR's existing labels, so that we don't accidentally remove any labels.

If it looks good to you, merge at will.

@phillipj
Copy link
Member Author

Forgot to mention I tested it with three PRs (9 - 11) in the test repo https://github.com/TestOrgPleaseIgnore/node/pulls

@Fishrock123
Copy link
Contributor

Seems good to me, let's try it imo.

@phillipj phillipj merged commit 79f828c into master Apr 16, 2016
@phillipj phillipj deleted the auto-tag-core-pr branch April 16, 2016 04:46
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