[PATCH 04/10] models: Add 'status' model

[Finucane, Stephen]

> On Wed, Sep 23, 2015 at 10:14:05PM +0100, Finucane, Stephen wrote:
> > > So it does mean my assumption was correct, eg. 'intel/license-check'
> > > will be repeated for every patch. Hence abstracting a Test table we can
> > > use for good.
> >
> > Yes, but the context does not imply that the tests are the same.
> > Please refer to the GitHub documentation for this. The non-uniqueness
> > of this is important.
> >
> > https://developer.github.com/v3/repos/statuses/
> 
> Oh, so you copied/pasted the github API. Sure that *can* work, but it
> doesn't mean it is authority.

Not quite. I researched any and all patch review tools and systems with CI integration that I could find over the course of a couple of months. I developed a prototype system outside of patchwork to trial my ideas, which you can find here https://github.com/stephenfin/diffusion. In the course of this investigation I worked with GitHub, Gerrit, BitBucket, review.ninja, patchew, Codebrag, and many, many more. I found the GitHub API provided the easiest methods for CI system "hooks", hence making it the most viable option. This also has the advantages of providing a familiar API for people to develop against. I try to avoid "Not Invented Here" syndrome and reuse good designs where possible.

> What does "the non-uniqueness of this is important" mean? I'm saying
> that patch after patch, you'll still have
> "continuous-integration/jenkins" repeated. I see some value in
> extracting a Test object and having "statuses" refer to that test
> object.

You're basically normalizing on this one field so? There's no harm in duplicating individual fields and pulling this single field out into its own table will impact DB performance for larger projects (due to the extra JOINs etc.). If you think there is value in identifying all tests with a given context we could index that column?

Regarding the other fields detailed in the 'Test' model provided:

* project
  This can be found as part of 'Status.patch.project'
* description
  I presume this will be a description of the test? If so it's not needed as this data is already contained in the CI job. The description field in 'Status' is used to give a description of the tests status (i.e. why the test failed/passed) thus serving a different purpose.

> > You are also assuming the "admins" for patchwork are the ones who
> > would decide what tests are run. This is a dangerous assumption.
> 
> Why is that dangerous?

Because it makes patchwork maintainers the authority figures as far as testing goes. This goes against the open ethos of a public mailing list. While a project might have many maintainers it is unlikely that there will be more than a couple of people with patchwork admin privileges. On a large project (https://patchwork.kernel.org/) the patchwork maintainer will become a bottleneck and patchwork itself a hindrance rather than a help. It also assumes that patchwork is being used actively, rather than passively as a neat way to view patches. This, once again, is a dangerous assumption (http://dpdk.org/dev/patchwork/project/dpdk/list/). 

> > Finally, how would you deal with the case where changes to different
> > files would result in different tests?
> 
> Define the list of tests, skip them when not needed.

Who would decide which patches to skip - the CI? That sounds an awful lot like an edge case to me. If you're happy to let an external service decide which tests to skip then why not let it decide which tests to run?

I noticed that you dropped the section on using ZUUL, a tool that can do much of this "smart parallel testing" for us? Why did you do this?

> > > I don't think that a compelling argument. For instance, in that case,
> we
> > > could have a more explicit "skip" or "non necessary" state for tests
> not
> > > run and have more visibility on what the testing system actually does
> > > while still tracking testing completion.
> >
> > Again, patchwork should not be in charge of this. A developer or
> > developer-provided script decides what tests to run. Patchwork just
> > reports.
> 
> And I'm asking "why not?"

Patchwork is a tool to supplement mailing lists, not replace them. The mailing list should remain the authoritative resource. If you disagree with this, then perhaps patchwork is not the tool for you.

	http://jk.ozlabs.org/projects/patchwork/

However, because I want to come to an amicable resolution, I will mention that if you wanted to then you could upload results directly from a CI system using the XML-RPC interface.

> > I wasn't calling you stupid - I was trying to be friendly. Cop on.
> 
> That really doesn't go through.

I'm sorry - I shall stick to English and curtail my use of emoticons henceforth.

> > I'm following the proposal by Thomas Monjalon of 6WIND. This series
> > works for that proposed model while yours does not. This has been
> > agreed on the mailing list. If you have issues, I suggest taking it up
> > in the original thread. Otherwise, can we please review this given in
> > light of that proposal?
> >
> > https://lists.ozlabs.org/pipermail/patchwork/2015-July/001363.html
> 
> Your cover letter should have mentionned it, I'd have started by that.

You're correct, it should have.

> --
> Damien

Given the complex requirements that you're specifying, Gerrit seems like a great fit. Why don't you use that? You seem to be trying to impose structure onto something that is inherently unstructured (even Gmail has a hard time threading mails reliably). It just seems like you're expecting patchwork to step well outside of its remit to fulfil a rather specific use case when there are more suitable tools already out there.