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

Try to find a comment to edit before creating #228

Merged
merged 5 commits into from
Jun 10, 2019

Conversation

refack
Copy link
Contributor

@refack refack commented Mar 26, 2019

  • Add @octokit/graphql

const getPRComments = `query getPRComments($owner: String!, $repo: String!, $number: Int!, $cursor: String){
repository(owner: $owner, name: $repo) {
pullRequest(number: $number) {
comments(first: 20, after:$cursor) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
comments(first: 20, after:$cursor) {
comments(first: 100, after:$cursor) {

I just put 20 to see if the recursive querying works.

}
return githubClient.issues.createComment({ owner, repo, number, body })
Copy link
Member

Choose a reason for hiding this comment

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

For sure like the looks of using GraphQL and promises 👍

nodes {
name
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Planning to use these labels for something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to recognize a bot-out label, for complete opt-out.

@phillipj
Copy link
Member

Leaning towards keeping these changes instead of what I suggested in #226, especially when it comes to talking to the GitHub API.

The other difference from the mentioned PR, is tests. You got any plans for more tests?

@refack
Copy link
Contributor Author

refack commented Mar 28, 2019

You got any plans for more tests?

That's my current WIP... mocking the GQL calls. (H/T to @gr2m for https://github.com/octokit/graphql.js/#writing-tests)

lib/github-graphql-client.js Outdated Show resolved Hide resolved
@gr2m
Copy link

gr2m commented Mar 28, 2019

Holler if you run into any blockers or have any questions about GitHub APIs / Octokit, happy to help

@phillipj
Copy link
Member

phillipj commented Apr 3, 2019

Any updates on this? I fully understand if you've got other things that needs attention -- if so, maybe we should consider merging #226 at first to reduce the amount of CI comment noise that @cjihrig was concerned about? 🤷‍♂️ ..then merge this as an improvement as soon as it's ready.

@phillipj
Copy link
Member

phillipj commented May 2, 2019

@refack would you mind if I dived into getting a test in place, with the goal of getting this merged & deployed?

@phillipj phillipj force-pushed the refine-pr-comment branch from 7212854 to 9e6f42c Compare May 5, 2019 20:07
@phillipj
Copy link
Member

phillipj commented May 5, 2019

@refack as I would really appreciate getting this landed soon, I took the freedom of rebasing with master to fix a package-lock.json merge conflict and pushed a couple of extra commits with missing GraphQL fixtures & tests -- sorry in advance if I crossed a line by doing that.

PTAL

@phillipj
Copy link
Member

phillipj commented Jun 5, 2019

@refack @cjihrig is this still something we want rolled out or are you happy with how things work currently?

@cjihrig
Copy link

cjihrig commented Jun 5, 2019

I'd still prefer if the bot limited its commenting when possible.

@phillipj phillipj force-pushed the refine-pr-comment branch from 8dfab1c to 09b360f Compare June 10, 2019 19:12
@phillipj
Copy link
Member

Alrighty then, let's give this a test round to see how it behaves 🚀

Rebased and force push to fix conflict in package-lock.json.

@phillipj phillipj merged commit 1cbae04 into nodejs:master Jun 10, 2019
@phillipj
Copy link
Member

This is now live in production.

phillipj added a commit to phillipj/github-bot that referenced this pull request Jun 15, 2019
Reverting changes related to editing existing comments, going back
to the previous behaviour of always creating new comments in PRs about
CI / Jenkins runs.

This is done primarily due to two things:

* It has broken node-core-utils' ability to check if CI's have been run
* There's not collaborator consensus about editing comments is actually
  better than creating new comments

Future plans for bot PR comments going forward has to be discussed more
thorougly and ensured it doesn't break other automation tools used
by collaborators.

Refs nodejs#228
phillipj added a commit that referenced this pull request Jun 15, 2019
Reverting changes related to editing existing comments, going back
to the previous behaviour of always creating new comments in PRs about
CI / Jenkins runs.

This is done primarily due to two things:

* It has broken node-core-utils' ability to check if CI's have been run
* There's not collaborator consensus about editing comments is actually
  better than creating new comments

Future plans for bot PR comments going forward has to be discussed more
thoroughly and ensured it doesn't break other automation tools used
by collaborators.

Refs #228
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.

4 participants