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

labels: fix labels not being added #87

Merged
merged 1 commit into from
Oct 19, 2016

Conversation

phillipj
Copy link
Member

After PR #80 got merged, no labels were added to new PRs.

That regression probably came from the fact that the existing repository labels was fetched and compared to the label names our labelling algorithm thought we should add. The problem is that we compared the whole label meta object (from api.github.com) against a label name (string).

Example of the two different types of objects:

// complete label object
{
  "url": "https://api.github.com/repos/nodejs/node/labels/buffer",
  "name": "buffer",
  "color": "f7c6c7"
}

// label name
"buffer"

Comparing two such objects will obviuosly never match.

These changes extracts the existing repo label names for comparison, and adds more logging for future debugging purposes.

Closes #86

/cc @mscdex

After PR nodejs#80 got merged,
no labels were added to new PRs.

That regression probably came from the fact that the existing repository
labels was fetched and compared to the label names our labelling algorithm
thought we should add. The problem is that we compared the whole
label meta object (from api.github.com) against a label name (string).

Example of the two different types of objects:

```
// complete label object
{
  "url": "https://api.github.com/repos/nodejs/node/labels/buffer",
  "name": "buffer",
  "color": "f7c6c7"
}

// label name
"buffer"

```

Comparing two such objects will obviuosly never match.

These changes extracts the existing repo label *names* for comparison,
and adds more logging for future debugging purposes.
phillipj

This comment was marked as off-topic.

phillipj

This comment was marked as off-topic.

@mscdex
Copy link
Contributor

mscdex commented Oct 19, 2016

LGTM if CI is ok with it.

@phillipj phillipj merged commit 8fda0ca into nodejs:master Oct 19, 2016
@phillipj phillipj deleted the fix-cached-labels branch October 19, 2016 18:36
@phillipj
Copy link
Member Author

This just got deployed.

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.

2 participants