Closed Bug 1446768 Opened 7 years ago Closed 6 years ago

Only post "No taskcluster jobs.." to a PR once

Categories

(Taskcluster :: Services, enhancement, P4)

Product:
Taskcluster
Component:
Services
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dustin, Assigned: ialokkumarsingh0, Mentored)

Details

(Keywords: good-first-bug)

We have a bunch of contributor PRs where every second comment is "No taskcluster jobs were created..". That's a little much. Ideally: * This error could be disabled completely with a flag in .taskcluster.yml in the master branch (similar to allowPullRequests) * When enabled, this error would only occur once in a PR; perhaps tc-github can search the comments on the PR for a similar comment and only post a new one if not found.
Mentor: dustin
Hey! May I work on this? Thank you.
Sure! I don't have a clear idea of how difficult it will be, but you are welcome. Irene and yd (both copied) have worked on this service before and may be able to help.
Thank you!:) Yes, I'll discuss with them to figure it out.
Priority: -- → P4

Could be done with a checkrun

Keywords: good-first-bug

Can I work on this bug?
Need some guidance before I can work on it though.

Flags: needinfo?(dustin)

Sure! Irene can help..

Flags: needinfo?(dustin) → needinfo?(bugzeeeeee)

My pleasure! Vaibhav, I don't know your level of experience, so let me know if I'm being too basic.

So this bug is for Github component. Our repository is here - please fork it. The source code for the Github component you'll find in services/github. A good start would be to look at the src/handlers.js file, find the code which is responsible for leaving the comment in question, understand how that works and see if you can come up with a solution or a question for me.

If you have any questions, you can ask them here (check "Need more information from" and type in "owlish" - it will find me. You can also ping me in #taskcluster-contributors channel on IRC (I saw you're already there), my nick is owlish.

Flags: needinfo?(bugzeeeeee)
Mentor: bugzeeeeee
Component: Github → Services
Assignee: nobody → ialokkumarsingh0

owlish: I saw the handler.js file and found that https://github.com/taskcluster/taskcluster/blob/master/services/github/src/handlers.js#L560 this block of code is responsible for comment "No Taskcluster jobs started for this pull request". I also saw the code in .taskcluster.yml file(I understand some stuff but not all) as comment by @dustin we can handle this problem by, before creating comment we check first whether this type of comment is made or not(with the help of flag(as suggested by @dustin))). If not create comment and if yes we can abort this process so that there is only comment present on the pr. what do you think?

Flags: needinfo?(bugzeeeeee)

I also saw the code in .taskcluster.yml file(I understand some stuff but not all) as comment by @dustin we can handle this problem by, before creating comment we check first whether this type of comment is made or not(with the help of flag(as suggested by @dustin))). If not create comment and if yes we can abort this process so that there is only comment present on the pr.

I'm not sure I understand this, what do you mean? Do all words "comment" mean the "No taskcluster jobs were created.." comment (the subject of this bug), or do they mean comment left by Dustin somewhere? Also, I'm not sure I know what has Dustin suggested - can you give a link to that suggestion or quote it here so that I understand what we're talking about? Thanks!

Flags: needinfo?(bugzeeeeee) → needinfo?(ialokkumarsingh0)

All comment words mean "No taskcluster jobs were created.." comment (the subject of this bug)". only "as comment by @dustin" which I previous mention was for the first comment made by dustin on this bug i.e

`
We have a bunch of contributor PRs where every second comment is "No taskcluster jobs were created..". That's a little much. Ideally:

  • This error could be disabled completely with a flag in .taskcluster.yml in the master branch (similar to allowPullRequests)

  • When enabled, this error would only occur once in a PR; perhaps tc-github can search the comments on the PR for a similar comment and only post a new one if not found. `

Flags: needinfo?(ialokkumarsingh0) → needinfo?(bugzeeeeee)

OK, so I understand you looked at handlers.js and identified the code area that needs to be worked on; you also looked at .taskcluster.yml; and you agree with Dustin's plan laid out in the first comment.

Dustin and I talked a bit about this, and decided that we are going to make some changes into the original plan. The first part stays the same:

This error could be disabled completely with a flag in .taskcluster.yml in the master branch (similar to allowPullRequests)

For the second part, instead of searching for the comments on PRs and checking whether we have posted or not, we are going to leave these comments in the event of opening a PR only. No other event should trigger this logic. Because currently all the PR events trigger commenting (synchronize, reopen etc) - and that is the reason of duplication.

So what we are going to do - we are going to check if the event we've got is a "PR", and if it is - check if we should start tasks (this is what we are doing currently). In case we should not start tasks, we check if that's a "PR opened" event, and if it is - we leave a comment; if it's not - we don't do anything (this will be added by you).

Finally, there will be a third part to this task: The checks that I just describe are happening very much in the middle of the function. So we do a whole bunch of work we don't need to do if this PR is not allowed. So the third part will be to take this whole piece and move it somewhere closer to the beginning of the function.

So that's the outline of the work. Let me know if you have any questions!

Flags: needinfo?(bugzeeeeee)

I made a commit regarding this issue which might solve part2 and part3. can you make some comment on it, if I am in the right direction or not? you can see my pr by going to this link.

Flags: needinfo?(bugzeeeeee)

Thank you! I reviewed

Flags: needinfo?(bugzeeeeee)
Status: NEW → ASSIGNED

@ialokkumarsingh0 Sorry for the pause - I ended up refactoring a bit of related old code in tc-github. Now that's done and deployed, we can continue and implement the part 1 - adding a flag to .taskcluster.yml which would allow people to disable the commenting completely. Let me know if you are interested in continuing on this ticket.

Flags: needinfo?(ialokkumarsingh0)

@owlish yes I am interested. how I can proceed can you give me some starter tips?

Flags: needinfo?(ialokkumarsingh0) → needinfo?(bugzeeeeee)

Awesome!

So, we currently can have this setting in the yml:

policy:
  pullRequests: ...

Currently there are 2 possivle settings to replace the ...: collaborators and public. Let's add a third option:

policy:
  pullRequests: collaborators_quiet

In the code, we currently check if the policy is collaborators. Having the third option as above, we would instead check if the policy startsWith('collaborators'). Also, we will change the check that helps us determine whether we need to leave a comment: we would leave a comment only if the event is pull-request.opened and the policy is collaborators.

We also will need to update the documentation for .taskcluster.yml v1 with the explanation of collaborators_quiet

Please let me know if you have any questions!

Flags: needinfo?(bugzeeeeee)

We just merged the PR (oops, that fell between the cracks!). Can you also make an update to the documentation about this new policy?

Flags: needinfo?(ialokkumarsingh0)

yes! where I can add this documentation for this.

Flags: needinfo?(ialokkumarsingh0) → needinfo?(dustin)

*the documentation

I searched a lot but didn't find any documentation file where I can update the documentation for this policy :(

thanks :)

Are you still working on this?

Flags: needinfo?(ialokkumarsingh0)

This issue has been resolved. You can close this :)

Flags: needinfo?(ialokkumarsingh0) → needinfo?(coop)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(coop)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.