[PULL] github.com/stephenfin/development

[Finucane, Stephen]

> On Thu, Oct 15, 2015 at 11:46:30PM +0000, Finucane, Stephen wrote:
> > Hey,
> >
> > First pull request (request-pull style, that is). Hope I've done
> > everything correctly :S This PR includes two fixes and the code enable
> > the check API. It's got positive reviews first time round and I
> > haven't heard any complaints for this revision so we're good to go.
> 
> My main concern is still the split test/testresult. I really think
> having separate objects in the database is more future proof.
> 
> For instance what if we want to select sending result emails for
> specific tests? there are plenty of properties like that that belong to
> a test object, not a test result object.
> 
> Another problem in this current model is knowing how many tests there
> are. If I have 20 patches, 5 tests and want an email with results, I'd
> rather not have 100 mails back, just one with consolidated results.
> 
> Your main objection to the table split was that you didn't want a single
> point of administration for adding tests. It doesn't have to be that
> way, a policy could be that tests rows can be created dynamically for
> instance and even keep the same front facing API. We could also empower
> maintainers of the project to add those tests, not the patchwork admin.
> In general, not just for this, we need member/maintainer roles in the
> project.
>
> Another thing that is a bit overlooked I believe is access control to
> posting test results?
>
> --
> Damien

Hey Damien,

Thanks for the comments: appreciate you getting back to me.

Let's compare the two a little further.

Your proposal:

	class Test(...):
	    name
	    description
	    project_id
	
	class PatchTestResult(...)
	    test_id
	    patch_id
	    result (the list of 'state choices' you had)
	    result_url

The existing proposal (w/ variables renamed for easy comparison):

	class Check(...):
	    patch_id
	    user_id
	    date

	    result
	    result_url
	    description
	    context

Your proposal denormalized:
	
	class PatchTestResult(...)
	    test_id
	    patch_id  # we can drop 'project_id' since patches are tied to a project

	    result (the list of 'state choices' you had)
	    result_url
	    description
	    name

When denormalized they are basically the same thing, right? So the argument becomes "Should we normalize the context/name field". I don't think we should because we want the amount of tests used to grow (or shrink) organically [1]:

	The goal is to be really open and distributed. It means new tests can spawn and
	some of them may be removed or disabled without central management. It allows
	to leverage a community for testing without management issues.

You appear to be insisting there should be a more static configuration of "runnable tests". However, I don't think this would scale or allow the fluidity of Thomas' proposal. In the normalized case, allowing a new CI system to start reporting test statuses would require (a) allowing them access to the mailing list and (b) configuring patchwork to enter this new tests. This, to me, is a barrier.

I hadn't planned on letting patchwork send any more emails, if I'm being totally honest. If you think an email would be useful, could we develop a script that checks for a given list of 'contexts' on a patch and asserts that those contexts all exist and have a 'passed' state. That would provide the functionality that you want while maintaining the dynamism that the community needs?

Look forward to your reply,
Stephen

[1] https://lists.ozlabs.org/pipermail/patchwork/2015-September/001706.html