Events for new comments for a patch series?

[Stephen Finucane]

On Thu, 2018-01-04 at 12:31 -0500, Don Zickus wrote:
> On Thu, Jan 04, 2018 at 02:30:04PM +0000, Stephen Finucane wrote:
> > On Mon, 2017-12-18 at 16:23 -0500, Don Zickus wrote:
> > > Hi,
> > > 
> > > I was playing with the events REST API interface and realized how
> > > convenient the 'series-completed' category is.
> > > 
> > > I was curious if there is also a way to be notified if a new
> > > comment
> > > arrived for a patch series.  We tend to use replies to patches as
> > > tags for meta info.  For example, acked-by, nacked-by, and also
> > > Bugzilla. There are times when a patch submitter forgets to add a
> > > Bugzilla id or another id is added because the patch solves
> > > another
> > > person's problem too.
> > > 
> > > Instead of reposting the patch series with the updated info, we
> > > tend
> > > to parse the comment and update some internal tags that is
> > > connected
> > > with the patch.
> > > 
> > > I was thinking, if having a 'events' interface that notified us a
> > > certain patch series had 'new comments', our polling script could
> > > retrieve and parse those comments.
> > 
> > That sounds like something we could do. I did this in the initial
> > version of the feature because I didn't think comments were
> > valuable
> > enough to expose. Perhaps I've called that wrong though. How would
> > this
> > compare with the Bugzilla-as-a-tag idea you've been discussing
> > elsewhere though?
> 
> Hi Stephen,
> 
> Thanks for the response.  I think the bugzilla tag email I sent,
> covers alot of the stuff I wanted to do.  I didn't fully understand
> how tags auto-generated from comments when I initially sent this
> hence the different approach.
> 
> > 
> > > Or maybe simpler, just a 'comments' REST API that when combined
> > > with project and since could achieve the same thing.
> > 
> > Why not both? '/events' is mostly there as an ease-of-use thing, as
> > it represents the one place that one can find out about all the
> > things going on in the system. There's no reason we couldn't do,
> > for example, '/patches/{patchId}/comments' though.
> 
> So I would like to add some context because creating a comments api
> could still be valuable to us, I am not sure.
> 
> Our internal forked instance of Patchwork tracks patches but more
> importantly tracked all emails on the mailing list.  The reason for
> this expansion into tracking all emails was to validate our
> automation was not broken.
> 
> Now for patchwork2.0 that just means don't drop any emails not
> related to patches.
> 
> Combining that idea with the comments api above, we had an objective
> that every email on the list was tagged was something (ideally acked-
> by, nacked-by, reviewed-by, etc).
> 
> Then we could manually process any comments that were _not_
> tagged.  Now for a public mailing list that is not strict with patch
> reviews this is a little obnoxious.
> 
> However, internally, we soon discovered broken emailers that could
> not thread replies correctly (thus we lost valuable patch
> reviews).  We would find mistyped acks (Akced-by or something).  Or
> more importantly, a patch would have multiple acks but someone would
> question the patch (not outright nack the patch but had some
> concerns).
> 
> The mistyped acks led to delayed patch acceptance and grumpy
> develoers.  The acked patch that had a question led to grumpy
> developers who wanted their questions answered before patch
> acceptance.
> 
> In order to capture this in a patch ready script, we decided to tag
> all comments with something (acked, nacked, reviewed) or needinfo
> (for questions) or needinfo_replied (for responses).  That way if a
> patch had any comments not tagged if would get held up.  Or if the
> patch had a 'needinfo' unresponded, it would get held up.
> 
> Again, probably overkill for a public mailing list with less strict
> patch reviews, but that is the motivation for asking about the
> comments api.
> 
> I think it is possible to expand the taggingn and comment apis and to
> customize our patch ready script thus allowing us to accomplish our
> needs without polluting everyone else's needs.
> 
> Does that make sense?  Thoughts?

That all makes sense. Based on the description above, it does sounds
like the tags feature will accomplish most of this for you. The only
issue we do have is that tags aren't fully "captured" - we only care
about the left-hand side. For example, take the two tags below:

  Reviewed-by: Stephen Finucane <stephen at that.guru>
  Reviewed-by: John Doe <john.doe at example.com>
  Tested-by: John Doe <john.doe at example.com>

All that we store (and expose) is the counts for each, which would look
 something like this.

  "tags": {
    "Reviewed-by": 2,
    "Tested-by": 1
  }

I'm guessing that what you actually want is something like this:

  "tags": {
    "Reviewed-by": [
      "Stephen Finucane <stephen at that.guru>",
      "John Doe <john.doe at example.com>"
    ],
    "Tested-by": [
      "John Doe <john.doe at example.com>"
    ]
  }

If so, we need to do a little work on this. This is captured (in rough
form) in https://github.com/getpatchwork/patchwork/issues/57. It's
definitely fixable, but I'm just not sure when I'm personally going to
have the time to do so. If it's useful enough, perhaps this is
something you folks could pick up?

Cheers,
Stephen

PS: In case I hadn't already mentioned, the design we've put in place
for cover letters should make storing non-patch emails rather easy for
you folks. I'd recommend looking at 'patchwork.models.Submission' for
more.