Re: [PULL] github.com/stephenfin/development
On Fri, Oct 16, 2015 at 07:08:45PM +0100, Finucane, Stephen wrote: > # 1) Add additional project configuration options: > > We should add the following options to the 'Project' model > > * Add an optional list property to specify the tests contexts that > have to run in order for a patch to be marked as "testing complete". > If set, an email will be sent on test completion. If unset, no emails > will be sent and Thomas' process can be used > * Add a Boolean property to state whether to restrict results to > these contexts only or whether to allow any and all contexts (but > basically ignore the ones not in the list) > * Add a Boolean property to restrict publishing of 'checks' to > maintainers or to all patchwork users. This can be expanded on later > (see below) > * [OPTIONAL] Add an option to turn 'Check' support on/off. Some > people won't want the extra column in the summary field cluttering > their view. This should be set to 'enabled' (i.e. use 'Checks') by > default, IMO. So now, I want to add the feature I already talked about: configure how result emails should be sent: - static tests like checkpatch.pl or sparse: only send emails on failure - long running tests (hours, 10s of hours) like piglit[1] or intel-gpu-tools[2]: always send result emails How would that fit in this model? I still think real test objects, disregarding how it's created, are the way to be more future-proof. However, I'm afraid I don't really have the time to discuss this any further. So don't let me block anything. -- Damien [1] http://cgit.freedesktop.org/piglit/ [2] http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/ ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
RE: [PULL] github.com/stephenfin/development
> On Fri, Oct 16, 2015 at 04:11:50PM +0100, Finucane, Stephen wrote: > > 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. > > I'm insisting it's *possible* to have a list of pre-defined tests, not > that it would be the only way to report test results. I can see merits > in both policies. I actually don't care how it's implemented as long as > it's reasonable and integrated into patchwork. > > > In the normalized case, allowing a new CI system to start reporting > > test statuses would require (a) allowing them access to the mailing > > list > > Nop. The test results are sent to patchwork, which then replies on > behalf of the tester. Several configurable policies could be possible, > depending on what project want to do: > > - send a consolidated email if we have a list of pre-defined tests > - send per-test mails otherwise? > - send consolidated mails on full series (remember the discussion > about testing full series because it can impractical to do it on > every patch, not just a corner case, we need it for the project I > work on) > - ... > > No need for the test system to be on the mailing list, no mail setup on > the client side is all win to me. > > > and (b) configuring patchwork to enter this new tests. This, to me, is > > a barrier. > > This doesn't have to be the case. Configurable behaviours are totally > fine. > > > 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. > > Given that patchwork is there to augment mails & mailing-lists, it > seemed natural to me to have it replying to patches with test results. > > An external script could mostly work. However I'd like it to be > integrated so we can do things like replying to the cover letter of the > series, which needs full access to the patchwork. OK, I can see the benefits of this scenario in some cases: let's add the functionality that you need without having to compromise on what other groups need. If so, I propose adding the following changes to the existing model. These can and should all be done in follow up series to keep each series as small and modular as possible. # 1) Add additional project configuration options: We should add the following options to the 'Project' model * Add an optional list property to specify the tests contexts that have to run in order for a patch to be marked as "testing complete". If set, an email will be sent on test completion. If unset, no emails will be sent and Thomas' process can be used * Add a Boolean property to state whether to restrict results to these contexts only or whether to allow any and all contexts (but basically ignore the ones not in the list) * Add a Boolean property to restrict publishing of 'checks' to maintainers or to all patchwork users. This can be expanded on later (see below) * [OPTIONAL] Add an option to turn 'Check' support on/off. Some people won't want the extra column in the summary field cluttering their view. This should be set to 'enabled' (i.e. use 'Checks') by default, IMO. # 2) Add additional 'reporter' or 'bot'-type user: Currently basic patchwork user rights are needed to submit a check. We could enforce this to allow maintainer-only rights (as stated above) but it would be better to add an additional 'reporter' role that would be allowed to publish results but not change the 'state' of a patch or do anything else. A bit like a daemon/service user in Linux, I guess. What do you think? Is this a good compromise that solves your needs? If so, I don't think there is a great reason to normalize the Check model for one field so let's keep it as is. Let's get this existing work merged and I'll submit the work for the project configuration options by early next week. These two series above will require modifications to the 'Project' and 'User' models respectively and won't take me long (I've already started on it). I should have it ready for review by the middle of next week. Cheers, Stephen ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PULL] github.com/stephenfin/development
On Fri, Oct 16, 2015 at 04:11:50PM +0100, Finucane, Stephen wrote: > 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. I'm insisting it's *possible* to have a list of pre-defined tests, not that it would be the only way to report test results. I can see merits in both policies. I actually don't care how it's implemented as long as it's reasonable and integrated into patchwork. > In the normalized case, allowing a new CI system to start reporting > test statuses would require (a) allowing them access to the mailing > list Nop. The test results are sent to patchwork, which then replies on behalf of the tester. Several configurable policies could be possible, depending on what project want to do: - send a consolidated email if we have a list of pre-defined tests - send per-test mails otherwise? - send consolidated mails on full series (remember the discussion about testing full series because it can impractical to do it on every patch, not just a corner case, we need it for the project I work on) - ... No need for the test system to be on the mailing list, no mail setup on the client side is all win to me. > and (b) configuring patchwork to enter this new tests. This, to me, is > a barrier. This doesn't have to be the case. Configurable behaviours are totally fine. > 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. Given that patchwork is there to augment mails & mailing-lists, it seemed natural to me to have it replying to patches with test results. An external script could mostly work. However I'd like it to be integrated so we can do things like replying to the cover letter of the series, which needs full access to the patchwork. -- Damien ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
RE: [PULL] github.com/stephenfin/development
> On Fri, Oct 16, 2015 at 03:17:42PM +0100, Finucane, Stephen wrote: > > > On Thu, Oct 15, 2015 at 11:46:30PM +, 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. > > > > > > This conflicts with the pull request I sent two weeks ago, I'm afraid. > > > > There didn't seem to be any movement on that pull request so I figured I > should push on. However, your series is there longer and therefore takes > priority: > > > > https://lists.ozlabs.org/pipermail/patchwork/2015-October/001802.html > > > > I'll get that series merged first then rework this PR. > > > > Hope this is OK? > > You are sending a pull request with unreviewed patches that introduce db > schema changes and new APIs. I don't think we should do that. It's quite > cheeky to present the work as ok because the last round didn't get any > replies (but wasn't addressing the concerns either). > > The elephant in the room is that the proposed db schema cannot handle > the use case I want to cover, while my modification still handles the > policy you'd like. I addressed the feedback from Thomas Monjalon and Thomas F Herbert, who gave me meaningful feedback and stayed around to address my comments. You, on the other hand, will neither review my patches (either at all or a meaningful way[1]) nor reply to my comments[2][3], Damien. Please tell me how else you expect me to get things merged in this environment? You expressed frustration about the lack of replies to your own patches but what about me: I review and test almost every single patches you submit, regardless of how big they are [4], and yet you don't return the favour. Please be a good community partner and help me out here or I'll have no choice but to go around you. Stephen [1] https://lists.ozlabs.org/pipermail/patchwork/2015-October/001880.html [1] https://lists.ozlabs.org/pipermail/patchwork/2015-September/001710.html [2] http://wiki.qemu.org/Contribute/SubmitAPatch#Pay_attention_to_review_comments [3] http://wiki.qemu.org/Contribute/SubmitAPatch#Make_code_motion_patches_easy_to_review ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
RE: [PULL] github.com/stephenfin/development
> On Thu, Oct 15, 2015 at 11:46:30PM +, 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
Re: [PULL] github.com/stephenfin/development
On Fri, Oct 16, 2015 at 03:17:42PM +0100, Finucane, Stephen wrote: > > On Thu, Oct 15, 2015 at 11:46:30PM +, 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. > > > > This conflicts with the pull request I sent two weeks ago, I'm afraid. > > There didn't seem to be any movement on that pull request so I figured I > should push on. However, your series is there longer and therefore takes > priority: > > https://lists.ozlabs.org/pipermail/patchwork/2015-October/001802.html > > I'll get that series merged first then rework this PR. > > Hope this is OK? You are sending a pull request with unreviewed patches that introduce db schema changes and new APIs. I don't think we should do that. It's quite cheeky to present the work as ok because the last round didn't get any replies (but wasn't addressing the concerns either). The elephant in the room is that the proposed db schema cannot handle the use case I want to cover, while my modification still handles the policy you'd like. -- Damien ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
RE: [PULL] github.com/stephenfin/development
> On Thu, Oct 15, 2015 at 11:46:30PM +, 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. > > This conflicts with the pull request I sent two weeks ago, I'm afraid. There didn't seem to be any movement on that pull request so I figured I should push on. However, your series is there longer and therefore takes priority: https://lists.ozlabs.org/pipermail/patchwork/2015-October/001802.html I'll get that series merged first then rework this PR. Hope this is OK? Stephen > -- > Damien ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PULL] github.com/stephenfin/development
On Thu, Oct 15, 2015 at 11:46:30PM +, 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 ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PULL] github.com/stephenfin/development
On Thu, Oct 15, 2015 at 11:46:30PM +, 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. This conflicts with the pull request I sent two weeks ago, I'm afraid. -- Damien ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
[PULL] github.com/stephenfin/development
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. Stephen --- The following changes since commit 634d97275f152470973996c242c9668bb53dff9f: login: Focus the username field on load (2015-09-18 16:49:26 +0100) are available in the git repository at: g...@github.com:stephenfin/patchwork.git development for you to fetch changes up to aaf79bb65a69b849af0fb83fab82ba1754cc6aa0: pwclient: Integrate 'checks' endpoint (2015-10-16 00:37:03 +0100) Stephen Finucane (10): models: Resolve issues with Patch.state trivial: Cleanup of 'models.py' trivial: Cleanup of 'admin.py' models: Add 'check' model models: Add properties related to checks templates/patch-list: Add patch "checks" column templates/patch: Add check summary panel trivial: Clean up 'views/xmlrpc.py' views/xmlrpc: Add 'checks' endpoint pwclient: Integrate 'checks' endpoint Thomas Petazzoni (1): Highlight patches with Acked/Reviewed/Tested tags htdocs/css/style.css| 44 +++ lib/sql/grant-all.mysql.sql | 5 ++- lib/sql/grant-all.postgres.sql | 11 +++-- lib/sql/migration/016-add-check-model.sql | 6 +++ patchwork/admin.py | 50 ++--- patchwork/bin/pwclient | 14 ++ patchwork/migrations/0001_initial.py| 2 +- patchwork/migrations/0002_fix_patch_state_default_values.py | 19 patchwork/migrations/0003_add_check_model.py| 33 ++ patchwork/models.py | 351 -- patchwork/settings/base.py | 1 + patchwork/templates/patchwork/patch-list.html | 5 +++ patchwork/templates/patchwork/patch.html| 25 +++ patchwork/templatetags/patch.py | 24 +- patchwork/tests/test_checks.py | 162 patchwork/views/xmlrpc.py | 405 - 16 files changed, 888 insertions(+), 269 deletions(-) create mode 100644 lib/sql/migration/016-add-check-model.sql create mode 100644 patchwork/migrations/0002_fix_patch_state_default_values.py create mode 100644 patchwork/migrations/0003_add_check_model.py create mode 100644 patchwork/tests/test_checks.py ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
