> 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
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
