Skip to content
This repository has been archived by the owner on Jan 5, 2024. It is now read-only.

GitHub: General, Ansibullbot & Scaling issues #357

Closed
gundalow opened this issue Sep 20, 2018 · 43 comments
Closed

GitHub: General, Ansibullbot & Scaling issues #357

gundalow opened this issue Sep 20, 2018 · 43 comments
Assignees
Labels
contributor_experience https://github.com/ansible/community/wiki/Contributor%20Experience

Comments

@gundalow
Copy link
Contributor

gundalow commented Sep 20, 2018

I should be able to make contact with people in the (platform /API developer experience) team(s) at GitHub. Need to think what to ask them

  • Issues with scaling Ansibullbot
    • Was chatting internally about this. One of the things we've been told before is to move to hook/event based bots. Though that doesn't work for us
      1. hooks are not reliable
      1. collector has to be able to process them quick enough
      1. Ansibullbot takes a while to effectively pull in all the relevant data to calculate state for a given issue/PR
      1. There isn't a hook sent when (say) a Shippable-ci Job is restarted - we have to poll
      1. all of our "stale" processes don't work either in a purely hook based flow
  • Do they have any other advice for dealing with large backlogs
  • Comments on rebasing, resolving conflicts - Can't use UI as it adds $something
  • Let them know the issues we have to work around
  • What have similarly sized groups have done
  • Can we set branch protections, user permissions at the GH Org level, rather than per repo?

This is just a place so we can collect ideas

@gundalow gundalow added the contributor_experience https://github.com/ansible/community/wiki/Contributor%20Experience label Sep 20, 2018
@mscherer
Copy link
Contributor

So, on the hook stuff, I think there is some workload that could be moved there. For example, the one that remind people we have a SIG, it doesn't change much if we lose it. I also wonder if we can get a hybrid approach, eg have a hook based workflow, and then run the bot less often or something like this ?

@mattclay
Copy link
Member

To make webhook handling more reliable, we could use a Lambda function to receive the webhooks and drop them into an SQS queue. The bot could consume the events from that SQS queue instead of needing to receive the directly.

@gundalow
Copy link
Contributor Author

From Tanner

  • Rate limiting
  • Real fields or components - selection box/drop down rather than free field

@webknjaz
Copy link
Member

@webknjaz
Copy link
Member

Though that doesn't work for us

python org bots fix this with adding delays after receiving hooks and then do some API queries

@webknjaz
Copy link
Member

Also, GitHub Apps integrations have a UI for checking on all the events they've sent to the integration along with a response from the service.

@sivel
Copy link
Member

sivel commented Sep 20, 2018

Though that doesn't work for us
python org bots fix this with adding delays after receiving hooks and then do some API queries

IIRC the problems were that even receiving the hook was unreliable, and we couldn't guarantee we would ever receive it

@webknjaz
Copy link
Member

Looks like an infra problem

@gundalow gundalow added this to Needs prioritizing in Community Sep 21, 2018
@gundalow gundalow changed the title GitHub Contact Ansibullbot & Scaling issues GitHub: General, Ansibullbot & Scaling issues Sep 21, 2018
@webknjaz
Copy link
Member

@gundalow tell gh that we're also unhappy with "GitHub Releases" feature not allowing to hide links to autogenerated git-archives sources, when overriding that with our proper assets.

@gundalow gundalow added this to Medium in Contributor Experience Sep 25, 2018
@gundalow gundalow moved this from Medium to To Triage in Contributor Experience Sep 26, 2018
@jctanner
Copy link
Contributor

Looks like an infra problem

Sometimes. It's also due to not every event triggering a webhook, such as shippable job re-runs or job-completes.

@gundalow
Copy link
Contributor Author

I have a meeting with one of the Product Managers at GitHub early next week, so if anyone has other comments please do add them.

@pabelanger
Copy link

pabelanger commented Jan 10, 2019

Allow a contributor to set approved for self code reviews, this is impossible today and forces the creation of a 2nd account to set approved.

@gundalow
Copy link
Contributor Author

Forms rather than freeform text for issue and PR creation to replace ISSUE TEMPLATES or PR TEMPLATES

@webknjaz

This comment has been minimized.

@GregSutcliffe
Copy link
Contributor

There's some kind of issue with the v3 API when requesting a large amount of PR information - I was doing some indexing of data, and requesting anything larger that "all PRs from May1st 2018 to now" resulted in errors. Details here and seems to affect other large repos (such as Kubernetes) too.

@pilou-
Copy link
Contributor

pilou- commented Jan 10, 2019

Allow people who don't have write access to ansible/ansible repository (GitHub contributors/maintainers in BOTMETA) to edit PR.

Reset all GitHub approvals when a PR is updated.

@webknjaz
Copy link
Member

@pilou-

Editing PRs is probably too risky and it's ACL control after all. I don't think it's possible.

Approvals reset can be configured on protected branches in GH settings.

@webknjaz
Copy link
Member

@GregSutcliffe anyway I think it's better to requests chunks rather than whole db...

@dagwieers
Copy link
Contributor

dagwieers commented Jan 10, 2019

How about an API to hide comments so we can make ansibot hide outdated or resolved comments. That would solve for me the biggest annoyance of ansibot (not cleaning up after himself).

As a result I am hiding irrelevant stuff in issues and PRs just to get a higher signal-to-noise and avoid the wall of text in some of the PRs.

This is now live:
You can now hide comments via GitHub's API:

ReportedContentClassifiers = OUTDATED https://developer.github.com/v4/enum/reportedcontentclassifiers/

@webknjaz
Copy link
Member

webknjaz commented Jan 10, 2019

+1 I also do that, it's sad that there's no API endpoint for hiding comments

This is now live:
You can now hide comments via GitHub's API:

ReportedContentClassifiers = OUTDATED https://developer.github.com/v4/enum/reportedcontentclassifiers/

@pilou-
Copy link
Contributor

pilou- commented Jan 10, 2019

Editing PRs is probably too risky and it's ACL control after all. I don't think it's possible.

Then it becomes "add ACL control".

Approvals reset can be configured on protected branches in GH settings.

It would be useful for unprotected branches too.

@dagwieers
Copy link
Contributor

Reset all GitHub approvals when a PR is updated.

I do not agree with this, there are many situations where an update to a PR should not reset approvals. An important one is a (required) rebase, but also smaller changes should not affect a prior approval. This really depends on the interpretation of an approval as well as the changes being made.

We would be shooting ourselves in the foot if we would reset approvals IMO. And it definitely does not improve Contributor Experience.

@dagwieers
Copy link
Contributor

Allow quick Github edits by people with commit rights. Currently it either means we edit the branch directly, or create a branch in the official project. I want to have the same option as users with read-access, create a branch in my personal space.

@webknjaz

This comment has been minimized.

@dagwieers
Copy link
Contributor

dagwieers commented Jan 10, 2019

Add more Github reactions, like one to indicate the commenter should have used Github reactions instead of adding a comment to approve a previous comment :-P

Maybe also a Github reaction to indicate a commenter should have read the fine documentation ?

Or a Github reaction to indicate you do not think it is a priority (rather than +1 and -1).

@webknjaz
Copy link
Member

webknjaz commented Jan 10, 2019

Regarding new Checks API thing. It doesn't affect us a lot but it will.
Checks listed in the bottom of PRs now add an extra click: they redirect one to the Check Suite details page however they often care about some of Check Runs failing.
This could be solved with a hovercard (like ones when you hover users' avatars) listing all check runs and maybe having a button next to each of them redirecting directly to that third-party service.

OTOH... It might be Travis CI misusing Checks API and dumping all jobs info to one page...

@dagwieers
Copy link
Contributor

dagwieers commented Jan 10, 2019

Automatically link users, issues and PRs in the Wiki (like how they work in comments).
Now I need to create individual links which is a drag.

@pilou-

This comment has been minimized.

@dagwieers

This comment has been minimized.

@pilou-

This comment has been minimized.

@alikins
Copy link

alikins commented Jan 10, 2019

One thing I'd like is a sidebar box with a list of other issues and pr's linked to the current issue/pr. Ideally 'above the fold' on the page. For an example, ansible/ansible#31751 is referenced by 3 other issues, and reference another issue and a project. But you have to read through all the comments to find it. Those references are easy to miss since they unconsciously get lumped in with the usually noisy issue event entries.

Hopefully that would help cut down on duplicate issues. And make the issue cross references more valuable.

And highlighting pull requests that reference an issue are more highlighted, it might increase the folks looking at or trying a pull request which can help it get resolved quicker.

A bonus step would be a page that summarizes the clumps of issues/prs that reference each other.
Maybe the N largest clumps/subgraphs that likely point to high value systemic flaws to address.

@bcoca
Copy link
Member

bcoca commented Jan 10, 2019

  • autolock issues older than X: we plan on adding this to bot since those get ignored anwyays, but it would prevent users from uselessly posting and feeling frustrated due to lack of answers on old closed tickets
  • fine grained commit access (subdirs): we do this with bot/shipit/botmeta .. but it would be much nicer as a GH feature
  • real forms (3rd mention, i know, but dropdowns/checkboxes would help soooo much).
  • a way to move comment into it's own ticket: cause no one ever posts unrelated issues on a ticket ...

@alikins
Copy link

alikins commented Jan 10, 2019

I would like to see a version of the 'Commits' log view that includes the full commit message. Both for the main repo view (https://github.com/ansible/ansible/commits/devel for example), and for the commits list in a PR. Even better would be a view with a 'git log --stat' equivalent.

Would make it easier for users to scan the commit history when looking for related changes.

For someone reviewing a PR it would help surface info (the full commit messages) which is tedious to view until you go to squash and merge and see them concatenated together and have to manipulate them into a coherent single commit message.

Related... Show more of first line of the commit message, even if it is longer than 50 char even if it has to be line wrapped. Up to some reasonable limit (500 chars? 1000) at least. The current display for the first/summary line often reads like a mystery with the last page ripped out. Any thing to encourage better and more detailed commit messages would be useful to project users and developers.

@alikins
Copy link

alikins commented Jan 10, 2019

Support finding pull requests that affect particular files. Possibly as a pr search field.
A file name oriented list/view of prs (something like https://ansible.sivel.net/pr/byfile.html ) is very
useful for project developers and users. Particularly for something like ansible where a given person
may only be interested in a particular module or plugin.

For pull requests, that data should exist.

It would be much much more complicated to be able to search issues by file though it would be kind of amazing. It probably makes sense for the issue->files info to be determined by something tailored to the project via some integration point.

Ansible does issue->file mapping to some degree with ansibot (ansible/ansible#50509 (comment) for ex).

But it is not easy for users to make use of that data. An example use for ansible is a user who is seeing problems with the 'at' module. The options for finding existing issue reports in that case are very slim. An issue search for 'at module' finds 1209 issues, mostly spurious. A search for 'lib/ansible/modules/system/at.py' or 'at.py' finds two relevant open issues.

@dagwieers
Copy link
Contributor

@pilou- We can't use the Github review system anyway, because Github does not have the same workflows or knowledge as ansibot does. Github does not know who a maintainer is, or how many shipits work for a given PR. Obviously being able to do everything we do in ansibot in Github could be useful, but singling out a specific feature will not help (instead it could make it worse if it lacks the holistic view)

@pilou-
Copy link
Contributor

pilou- commented Jan 10, 2019

@dagwieers Being able to use GitHub review system approval status would make Ansibot simpler.

@felixfontein
Copy link
Contributor

In the diff view of a PR, longer sections between two diff positions are collapsed away. When clicking the expand button, lines from below the expand button are added. If one is interested in the lines below the diff above the expand button, one has to click the expand button very many times (depending on the distance to the next diff). That's very annoying.

Also, sometimes one wants to get rid of all collapsed positions to see the diff of the complete file, for example to see how a PR interacts with the total file. At the moment, this involves a lot of manual clicking. A button "expand all" per file (or even per PR) would improve this situation alot!

@webknjaz
Copy link
Member

This question needs to be addressed for reliability improvements in bots:

Long story short, they provide API to query events but there's no way to match them with events received via webhooks because they use different kinds of identifiers, so I can see the same events on the UI but there's no way to connect them 😞

I think that the solution is extremely simple on their side: just add another field to data in API or to headers in their webhook events.

@dagwieers
Copy link
Contributor

dagwieers commented Mar 28, 2019

It would be nice if you can easily go from a PR to the author's branch. Currently that branch is listed at the top as:

dagwieers wants to merge 2 commits into ansible:devel from dagwieers:file-locking

But it would be more convenient if you could click on dagwieers:file-locking to go to that branch (i.e. for testing/downloading that branch).

@webknjaz
Copy link
Member

@dagwieers it's actually what GitHub has implemented a few days ago 👍

@webknjaz
Copy link
Member

webknjaz commented Apr 2, 2019

@gundalow can we ask GitHub to allow buttons in Checks API to be available to everyone so that access control would be possible to do in the click handlers on GitHub App side.

@webknjaz
Copy link
Member

webknjaz commented Apr 2, 2019

@mkrizek mkrizek removed their assignment Apr 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
contributor_experience https://github.com/ansible/community/wiki/Contributor%20Experience
Projects
No open projects
Community
  
Needs prioritizing
Development

No branches or pull requests