Details
- Reviewers
olasd - Group Reviewers
Reviewers - Commits
- R154:aa87d8b236db: Add Phabricator feed announcement to IRC.
Diff Detail
- Repository
- R154 Limnoria plugins
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Thanks for the patch! Looks like a good first pass at the issue. I have a few inline comments that should be addressed before merging this.
The following are a few thoughts that came to mind, but are really refinements that can be implemented in further diffs rather than making this one a big pile of features
- I'm worried that this is going to be fairly noisy, in a few situations:
- automatic transactions, e.g. a new diff having a reviewer added by Herald isn't that interesting
- a push of a branch will notify of all commits separately; that's a problem when e.g. importing or updating an external repository
I'm not quite sure what combination of rate limiting and/or object/message filtering we should use to make it tenable.
- The polling interval can probably be decreased to a minute to avoid getting too many messages in bursts.
- It'd be nice to add clickable URLs to the objects that are being talked about (the phid.query endpoint should provide that).
- If we enable this on both swh-devel and swh-team, we will yield duplicate messages (as the public repos are a subset of what is visible on the private key). I guess this would be a case for having a "per-space" filtering strategy, but actually getting which space an object is in is not that easy (for each object type, it's a separate query).
| Phabricator/plugin.py | ||
|---|---|---|
| 452 | This should sort the stories by epoch before processing them. | |
| Phabricator/test.py | ||
| 42–195 | I think this label should be swapped with ENTRY3 to make sure they're in order. | |
| 222 | I'm assuming this is mocked by supybot.test and it's not actually sleeping for one second :-) | |
On Tue, Oct 2, 2018 at 6:20 PM olasd (Nicolas Dandrimont) <forge@softwareheritage.org> wrote:
a push of a branch will notify of all commits separately; that's a problem when e.g. importing or updating an external repository
I'm not quite sure what combination of rate limiting and/or object/message filtering we should use to make it tenable.
FYI, here's what we do in another org (I just checked):
- We filter all the commits whose sha1 we've already seen
- If there's a branch pushed with a single commit, we show the commit and a link to the commit
<bot> [swh-storage] seirl pushed b11c8b5 on ⊶branch_name: “storage: commit message” ⋅ https://short.url/
- If there's a branch pushed with multiple commits, we say "pushed N commits" and we show the name of the last one, and the URL takes you to a diff of all the changes (first_commit..last_commit of the push)
<bot> [swh-storage] seirl force-pushed 2 commits including b11c8b5 on ⊶branch_name: “storage: commit message” ⋅ https://short.url/
The spamming is pretty low that way, but maybe we don't have the same push density as SWH :-)
- Construct the story announce, allowing for sending clickable URLs to IRC.
- Decrease polling interval to a minute.
- Add username blacklist to hide bots (not all of them are marked as such in Phabricator's API).
Fixed by adding a username blacklist.
- a push of a branch will notify of all commits separately; that's a problem when e.g. importing or updating an external repository
That's a single story, therefore a single IRC message for all transactions of the story.
- The polling interval can probably be decreased to a minute to avoid getting too many messages in bursts.
Done.
- It'd be nice to add clickable URLs to the objects that are being talked about (the phid.query endpoint should provide that).
Done.
- If we enable this on both swh-devel and swh-team, we will yield duplicate messages (as the public repos are a subset of what is visible on the private key). I guess this would be a case for having a "per-space" filtering strategy, but actually getting which space an object is in is not that easy (for each object type, it's a separate query).
Indeed :/
| Phabricator/test.py | ||
|---|---|---|
| 42–195 | No, I'm using the original order given by the API for the tests. | |
| 222 | It's not mocked, but that's a very good idea. That will greatly speed up Limnoria's test suite. | |
Cool.
- a push of a branch will notify of all commits separately; that's a problem when e.g. importing or updating an external repository
That's a single story, therefore a single IRC message for all transactions of the story.
Ah, I guess that changed since I last looked. Cool!
- If we enable this on both swh-devel and swh-team, we will yield duplicate messages (as the public repos are a subset of what is visible on the private key). I guess this would be a case for having a "per-space" filtering strategy, but actually getting which space an object is in is not that easy (for each object type, it's a separate query).
Indeed :/
I think we can just enable it on the public channel for now and see where that takes us.
Thanks!
| Phabricator/plugin.py | ||
|---|---|---|
| 136 | I'm pretty sure transactions are supposed to be immutable and there won't be a need to ever refresh the cache. But the symmetry between both methods has some appeal :-) | |
| Phabricator/test.py | ||
| 42–195 | The API returns a JSON object with PHIDs as keys, I don't think there's a guarantee that entries are returned in any order by Phabricator, or that Python preserves this order (well, there may be in Python 3.7 now that dicts are ordered by spec, but that's beside the point). I really think it would be less confusing to see the entries 1, 2, 3 sorted chronologically in this test, but it is purely cosmetic. | |
| 222 | You may want to look at https://github.com/alisaifee/hiro which is one way of handling "time testing" sort-of-transparently | |