Rather than use Bugzilla Tweaks at all, which we would like to slowly phase out as we add more features to BMO directly, we propose creating a BMO extension called AutoLand. This would allow us to incorporate the needed functionality directly to BMO with the need of add-ons or javascript. Also using an extension, we can create a separate database table allowing us to store data for use by the WebService API calls and other workflow status. This will alleviate the need for using the whiteboard which could be prone to error if the user does not format it correctly or if someone adds to the whiteboard that should not be allowed to. Use Case: * A user that has patch(es) attached to a bug and wants to have the patch(es) autolanded will check the a checkbox next to each patch in the attachments table, they will enter any branches the patches should be applied to after a successful Try run by entering something like 'mozilla-aurora,mozilla-beta,mozilla-central' into a text box (only present when extension is enabled) and then submit this information to a database via an Autoland submit button. * The current AutoLand polling server, using a WebService API call will look for bugs that have patches AutoLand submitted on that are not yet marked as landed, will look for attachments that need processing. * The AutoLand server will push the patches to try, and if successful, commit to $branches if the autoland initiator has the proper permissions. A message will be sent back to the WebService API to update the status of this entry * The AutoLand server, once the process is completed, uses another WebService API call to update the status of the entry to complete so the autoland input UI can be unlocked for additional use * During each step of the process, try, hg commit, etc., the AutoLand server uses the Buzilla API (using their own tools to interface with it) to add comments to the bug report letting the user know the status of their patch(es). More details at http://wiki.mozilla.org/BMO/AutoLand

First iteration patch for review. Everything works but still needs some cleanup and documentation. Also few things to note: 1. The webservice methods are AutoLand.getBugs and AutoLand.updateStatus 2. Currently I am not tracking any history of the changes in bugs_activity and also therefore no email notifications are mailed showing any changes. Do we really need or care to have this tracked in the bug history? People will get notified when the Try server updates the bug with a comment upon completion anyway. 3. It was much easier to add the AutoLand checkbox the actions section of the attachments table than to hook in a new column. If this is not satisfactory I can make the other work. 4. The current accepted status list is waiting, running, success, failed. The call to AutoLand.updateStatus will need to provide one of those. Maybe some other things I cant remember. dkl

> 2. Currently I am not tracking any history of the changes in bugs_activity > and also therefore no email notifications are mailed showing any changes. Do > we really need or care to have this tracked in the bug history? I don't think we need this, in fact it would be a bonus _not_ to have a bunch of history/email created by autoland since a single bug may go through many autolanding runs

Comment on attachment 596197 [details] [diff] [review] Patch to add AutoLand extension to BMO (v1) Review of attachment 596197 [details] [diff] [review]: ----------------------------------------------------------------- nice, most of this code is looks and works well :) -- your boilerplate for all files is wrong, remove the perlmode line, and contributors. it should just be: +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +# +# This Source Code Form is "Incompatible With Secondary Licenses", as +# defined by the Mozilla Public License, v. 2.0. with no extra lines. -- the specs has: "When a patch has been checked for AutoLand, disable the checkbox so it cannot be manipulated until the AutoLand server has finished processing the patch." the checkbox is only disabled once it has been updated to 'running'; it should be disabled immediately to avoid race conditions. as far as i can see from the specs, once the status has been updated to failed or success, the checkbox unchecked. -- i'm not sure from the specs if branches should be per-patch, or per-bug. the implantation here has it as per-bug, and i'd like to get confirmation that this is correct. ::: extensions/AutoLand/lib/WebService.pm @@ +33,5 @@ > + > + $user->in_group('hg-try') > + || ThrowUserError("auth_failure", { group => "hg-try", > + action => "list", > + object => "bugs"}); you'll need to be more restrictive than just hg-try membership for this call. in its current form i could use it to get information about any attachment that has been pushed, even if i don't have access to that attachment (all it'd need is membership of hg-try). this call should be restricted to a single user, which only the autoland system itself uses. @@ +70,5 @@ > + return [ > + map > + { { bug_id => $_, attachments => $bugs{$_}{'attachments'}, branches => $bugs{$_}{'branches'} } } > + keys %bugs > + ]; nit: funky indentation @@ +88,5 @@ > + > + $user->in_group('hg-try') > + || ThrowUserError("auth_failure", { group => "hg-try", > + action => "update", > + object => "status"}); apply the same restrictions from getBugs here. ::: extensions/AutoLand/template/en/default/hook/attachment/list-action.html.tmpl @@ +19,5 @@ > + (<span class="autoland_[% attachment.autoland_status %]" > + title="Last Updated: [% attachment.autoland_status_when FILTER html %]"> > + [% attachment.autoland_status FILTER html %]</span>) > + [% END %] > +[% END %] i understand why you've done it this way, but it doesn't seem right to mix it in with the actions, and i would much prefer a new column, or some other presentation. perhaps another solution would be to add a new table just for autoland, listing only the applicable attachments? doing it this way will also let you bring the branched field closer to the autoload patch selections. or, given autoland actions are really outside of a normal bug update, another option would be to have a link on the bug near the attachments table which takes to you a custom page which displays the applicable attachments, enables branch selection, and submission. another option would be to drop the checkbox; make "autoland" look like a normal attachment action, and use javascript to kick off the process. immediately replace the action text with an unlinked "Autoland (waiting)" blurb, reverting back to an action once autoland flags it as failed/success. a downside of this would multi-patch autolanding would no longer be atomic, so a flagging/polling race condition would exist. (i'm not sure either of these work either, just thinking out loud). ::: extensions/AutoLand/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl @@ +15,5 @@ > + <label>AutoLand Branches:</label> > + </th> > + <td> > + <input type="text" id="autoland_branches" name="autoland_branches" > + value="[% bug.autoland_branches FILTER html %]" size="60"> this should be size=40 and have a text_input class.

(In reply to Byron Jones ‹:glob› from comment #3) > the specs has: "When a patch has been checked for AutoLand, disable the > checkbox so it cannot be manipulated until the AutoLand server has finished > processing the patch." > > the checkbox is only disabled once it has been updated to 'running'; it > should be disabled immediately to avoid race conditions. > > as far as i can see from the specs, once the status has been updated to > failed or success, the checkbox unchecked. My original thinking was someone might have checked it by mistake or changed their mind shortly after and would still have a chance to cancel it if it had not yet transitioned to 'running'. Lukas, do you have an idea on this? > i'm not sure from the specs if branches should be per-patch, or per-bug. > the implantation here has it as per-bug, and i'd like to get confirmation > that this is correct. Lukas, can you verify this slso please? Are we sure it is ok to have the branches value applicable for all patches attached to a single bug? Will there ever be a case where one or more patches with have to have different branch values that the others on a single bug? > ::: > extensions/AutoLand/template/en/default/hook/attachment/list-action.html.tmpl > @@ +19,5 @@ > > + (<span class="autoland_[% attachment.autoland_status %]" > > + title="Last Updated: [% attachment.autoland_status_when FILTER html %]"> > > + [% attachment.autoland_status FILTER html %]</span>) > > + [% END %] > > +[% END %] > > i understand why you've done it this way, but it doesn't seem right to mix > it in with the actions, and i would much prefer a new column, or some other > presentation. > > perhaps another solution would be to add a new table just for autoland, > listing only the applicable attachments? doing it this way will also let > you bring the branched field closer to the autoload patch selections. I actually like this idea. I could make it collapsable or just hide patches not yet selected and have a (more patches) link similar to the flags. I will work on something and post a new patch. dkl

New patch that addresses hopefully all of your changes. dkl

Comment on attachment 596831 [details] [diff] [review] Patch to add AutoLand extension to BMO (v2) Review of attachment 596831 [details] [diff] [review]: ----------------------------------------------------------------- i like this presentation much more than the initial patch :) can you please: - position it after the tracking flags fields - wrap it in a collapse block it would also benefit from a link to some documentation describing what it does and how to use it. ::: extensions/AutoLand/Extension.pm @@ +94,5 @@ > + my $dbh = Bugzilla->dbh; > + return $self->{'autoland_checked'} if $self->{'autoland_checked'}; > + $self->{'autoland_checked'} > + = $dbh->selectrow_array("SELECT 1 FROM autoland_attachments > + WHERE attach_id = ?", undef, $self->id); to save a few db trips, select who, status and when from autoland_attachments here, and populate $self->{autoload_who}, {autoland_status} and {autoland_status_when}. the getters for who, status and when all call ->autoland_checked first, so they can also be simplified. @@ +147,5 @@ > + my $bug_id = $object->bug_id; > + my $old_branches > + = $dbh->selectrow_array("SELECT branches FROM autoland_branches > + WHERE bug_id = ?", undef, $bug_id); > + my $new_branches = $cgi->param('autoland_branches') || ''; why switch from Bugzilla->input to cgi->param? not fussed either way, just wondering :) @@ +199,5 @@ > + my $vars = $args->{'vars'}; > + > + # in the header we just need to set the var to ensure the css gets included > + if ($file eq 'bug/show-header.html.tmpl') { > + $vars->{'autoland'} = 1; you only need to do this if the user is in the hg-try group ::: extensions/AutoLand/lib/WebService.pm @@ +71,5 @@ > + return [ > + map > + { { bug_id => $_, attachments => $bugs{$_}{'attachments'}, branches => $bugs{$_}{'branches'} } } > + keys %bugs > + ]; the indentation is wrong here, it should be: return [ map { ... ]; @@ +84,5 @@ > + > +sub updateStatus { > + my ($self, $params) = @_; > + my $user = Bugzilla->user; > + my $dbh = Bugzilla->dbh; nit: nuke extra spaces @@ +113,5 @@ > + if ($attachment && $attachment->isprivate && !$user->is_insider) { > + ThrowUserError('auth_failure', { action => 'access', > + object => 'attachment', > + attach_id => $attachment->id }); > + } you don't need to revalidate $attachment here. ::: extensions/AutoLand/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl @@ +11,5 @@ > + <th class="field_label"> > + <label>AutoLand:</label> > + </th> > + <td> > + Branches<br> "Branches:" @@ +25,5 @@ > + [% autoland_patches.push(attachment) %] > + [% END %] > + [% IF autoland_patches.size %] > + <br> > + Patches<br> "Patches:" @@ +53,5 @@ > + <span class="autoland_[% attachment.autoland_status FILTER html %]"> > + [% attachment.autoland_status FILTER html %] > + </span> > + [% END %] > + </td> i don't think listing the attachment id as the primary identifier is informative. i'd suggest showing the attachment description in the table, with the filename in the tooltip (or vice-versa).

(In reply to Byron Jones ‹:glob› from comment #7) > Comment on attachment 596831 [details] [diff] [review] > Patch to add AutoLand extension to BMO (v2) > > Review of attachment 596831 [details] [diff] [review]: > ----------------------------------------------------------------- > > i like this presentation much more than the initial patch :) > > can you please: > - position it after the tracking flags fields Not sure how do to control the order of how extension template hooks are processed. The tracking flags are also in an edit-after_custom_fields.html.tmpl just like my code. Do you know a way to guarantee order? > - wrap it in a collapse block Will do it similar to flags where it shows the patches running as a count with a (edit) link after displaying the full table. > it would also benefit from a link to some documentation describing what it > does and how to use it. I can create a template hook to the fields.html.tmpl table. > @@ +147,5 @@ > > + my $bug_id = $object->bug_id; > > + my $old_branches > > + = $dbh->selectrow_array("SELECT branches FROM autoland_branches > > + WHERE bug_id = ?", undef, $bug_id); > > + my $new_branches = $cgi->param('autoland_branches') || ''; > > why switch from Bugzilla->input to cgi->param? > not fussed either way, just wondering :) Switched back. At first it wasnt very apparent that input_params would give me the defined_autoland_attachments as an array ref like CGI.pm does but it worked out ok. process_bug.cgi uses CGI.pm for getting defined_groups. dkl

Patch with suggested changes. dkl

Comment on attachment 597298 [details] [diff] [review] Patch to add AutoLand extension to BMO (v3) Review of attachment 597298 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to David Lawrence [:dkl] from comment #8) > > can you please: > > - position it after the tracking flags fields > > Not sure how do to control the order of how extension template hooks are > processed. The tracking flags are also in an > edit-after_custom_fields.html.tmpl just like my code. Do > you know a way to guarantee order? it's done based on the name of the extension; and easy fix would be to rename the extension to sort after BMO, such as "TryAutoLand". i think it's important to get this right, because the tracking flags are more important than autoland, and shouldn't be shifted around. > > - wrap it in a collapse block > > Will do it similar to flags where it shows the patches running as a count > with a (edit) link after displaying the full table. please include all autoland ui within the collapse block, not just the attachments. something like: AutoLand: (edit) Total: 4 - Waiting: 3 - Running: 0 - Finished: 0 i really like the collapsed summary btw :) otherwise this is looking excellent. ::: extensions/AutoLand/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl @@ +41,5 @@ > + [% IF autoland_patches.size %] > + Patches: > + <span id="autoland_edit_container"> > + (<a href="#" id="autoland_edit_action">edit</a>) > + Total: [% bug.attachments.size FILTER html %] - you should only be showing a count of patches on the bug, not all attachments.

Thanks for the review. This should address your requested changes. Couple comments: 1) Renamed directory to TryAutoLand to make it display after BMO Tracking flags. This is by directory name only. Internally the extension is still known as AutoLand as well as the WebService calls. 2) Now requires a valid user login of 'autoland-try@mozilla.org' for both the AutoLand.getBugs and AutoLand.updateStatus calls. I just made this up, we can change it to something else if desired. 3) The branches form field and patches table are hidden unless a user clicks (edit). 4) There is a small tooltip that displays when the user mouses over the AutoLand field label. If the user clicks it it will take them to a more detailed help page (page.cgi?id=autoland.html). I will need Lukas/Marc to come up with some text for the page to help users understand how the integration works. I will install what I currently have on bugzilla-stage-tip.mozilla.org so testing can begin now. I will create the autoland-try@mozilla.org account and send the password in an email. dkl

Comment on attachment 597574 [details] [diff] [review] Patch to add AutoLand extension to BMO (v4) Review of attachment 597574 [details] [diff] [review]: ----------------------------------------------------------------- > 1) Renamed directory to TryAutoLand to make it display after BMO Tracking > flags. This is by directory name only. Internally the extension is still > known as AutoLand as well as the WebService calls. please make the perl package names should match the directory name, to avoid future confusion. the webservice calls and internal names can stay as autoland. > I will install what I currently have on bugzilla-stage-tip.mozilla.org so > testing can begin now. I will create the autoland-try@mozilla.org account > and send the password in an email. autoland-try@mozilla.org may trigger emails and will get extra permissions due to the domain name. we should use a domain which ends in .tdl which won't do either (such as autoland-try@mozilla.tld). https://wiki.mozilla.org/Build:Autoland states that with the current whiteboard-based system, patches can be applied in a specific order; this system doesn't support that. lsblakk: do you think this will be a problem? ::: extensions/TryAutoLand/Extension.pm @@ +112,5 @@ > +} > + > +sub _autoland_attachment_who { > + my $self = shift; > + my $dbh = Bugzilla->dbh; dbh is not used and can be removed @@ +119,5 @@ > +} > + > +sub _autoland_attachment_status { > + my $self = shift; > + my $dbh = Bugzilla->dbh; dbh is not used and can be removed @@ +126,5 @@ > +} > + > +sub _autoland_attachment_status_when { > + my $self = shift; > + my $dbh = Bugzilla->dbh; dbh is not used and can be removed ::: extensions/TryAutoLand/lib/WebService.pm @@ +29,5 @@ > + > + if ($user->login ne 'autoland-try@mozilla.org') { > + ThrowUserError("auth_failure", { action => "access", > + object => "autoland_patches" }); > + } move the login to Constants, and change to "autoland-try@mozilla.tld" or similar. @@ +89,5 @@ > + > + if ($user->login ne 'autoland-try@mozilla.org') { > + ThrowUserError("auth_failure", { action => "modify", > + object => "autoland_patches" }); > + } email const here too. ::: extensions/TryAutoLand/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl @@ +30,5 @@ > + [% END %] > + <tr> > + <th class="field_label field_land_autoland"> > + <a title="[% help_html.autoland FILTER txt FILTER collapse FILTER html %]" > + class="field_help_link" href="page.cgi?id=autoland.html"> as you noted, the autoland.html page wasn't included with this patch. you could simply link to https://wiki.mozilla.org/Build:Autoland instead. ::: extensions/TryAutoLand/template/en/default/hook/bug/field-help-end.none.tmpl @@ +15,5 @@ > + # Initial Developer. All Rights Reserved. > + # > + # Contributor(s): > + # Dave Lawrence <dkl@mozilla.com> > + #%] wrong boilerplate

This is looking good! In order to get testing started with this, I'll need the access information to the WebServices API on stage-tip, since https://api-dev.bugzilla-stage-tip.bugzilla.mozilla.org/latest/ seems to be down, or maybe I just have the address wrong.

(In reply to Marc Jessome[:mjessome] from comment #13) > This is looking good! Thanks > In order to get testing started with this, I'll need the access information > to the WebServices API on stage-tip, since > https://api-dev.bugzilla-stage-tip.bugzilla.mozilla.org/latest/ seems to be > down, or maybe I just have the address wrong. You will be able to use the BzAPI (REST) proxy for accessing this data as the proxy is not written to know about it and we do not maintain it ourselves. You will however be able to access it via XMLRPC, JSONRPC, or JSONP against https://bugzilla-stage-tip.mozilla.org directly. Let me know if you need example code for each. dkl

(In reply to Byron Jones ‹:glob› from comment #12) > > 1) Renamed directory to TryAutoLand to make it display after BMO Tracking > > flags. This is by directory name only. Internally the extension is still > > known as AutoLand as well as the WebService calls. > > please make the perl package names should match the directory name, to avoid > future confusion. the webservice calls and internal names can stay as > autoland. Dang, but I liked the name AutoLand better ;) I will change it to TryAutoLand for the webservice calls too as I want it to be consistent. We really need to add some sort of sorting value in the extensions (maybe Config.pm?) so that we don't rely on file sorting to dictate order (separate bug). > > I will install what I currently have on bugzilla-stage-tip.mozilla.org so > > testing can begin now. I will create the autoland-try@mozilla.org account > > and send the password in an email. > > autoland-try@mozilla.org may trigger emails and will get extra permissions > due to the domain name. we should use a domain which ends in .tdl which > won't do either (such as autoland-try@mozilla.tld). Changed to autoland-try@mozilla.bugs > https://wiki.mozilla.org/Build:Autoland states that with the current > whiteboard-based system, patches can be applied in a specific order; this > system doesn't support that. lsblakk: do you think this will be a problem? Suppose I could add a sorting input next to each checkbox (default 0) that users could specify order. Will take an hour or two work to add it. dkl

Thanks for the review. I think we're getting close :) Comments: 1) Renamed throughout to TryAutoLand to be consistent. 2) Updated help docs to point to wiki 3) Updated UI 4) New attachment->autoland_update_status instead of updating DB directly in TryAutoLand.updateStatus 5) Update constant for WEBSERVICE_USER to be autoland-try@mozilla.bugs 6) other requested fixes. Will install this on bugzilla-stage-tip.mozilla.org as well dkl

Comment on attachment 597966 [details] [diff] [review] Patch to add AutoLand extension to BMO (v5) r=glob i'm happy this as it stands; commends below can be taken or ignored :) >+ my $attachment = Bugzilla::Attachment->new($attach_id); >+ ($attachment && !$attachment->isobsolete) >+ || ThrowUserError('autoland_invalid_attach_id', >+ { attach_id => $attach_id }); i don't think the isobsolete check should be here -- if a try run has completed we may as well capture the results, even if someone has obsoleted the patch while it was running. >+ [% IF autoland_patches.size %] >+ Total: [% autoland_patches.size FILTER html %] - >+ <span class="autoland_waiting">Waiting:</span> [% autoland_waiting FILTER html %] - >+ <span class="autoland_running">Running:</span> [% autoland_running FILTER html %] - >+ <span class="autoland_success">Finished:</span> [% autoland_finished FILTER html %] >+ [% ELSE %] >+ No patches >+ [% END %] how about not showing any autolander stuff at all if there aren't any patches?

(In reply to Byron Jones ‹:glob› from comment #17) > >+ my $attachment = Bugzilla::Attachment->new($attach_id); > >+ ($attachment && !$attachment->isobsolete) > >+ || ThrowUserError('autoland_invalid_attach_id', > >+ { attach_id => $attach_id }); > > i don't think the isobsolete check should be here -- if a try run has > completed we may as well capture the results, even if someone has obsoleted > the patch while it was running. Good point. Fixed. > >+ [% IF autoland_patches.size %] > >+ Total: [% autoland_patches.size FILTER html %] - > >+ <span class="autoland_waiting">Waiting:</span> [% autoland_waiting FILTER html %] - > >+ <span class="autoland_running">Running:</span> [% autoland_running FILTER html %] - > >+ <span class="autoland_success">Finished:</span> [% autoland_finished FILTER html %] > >+ [% ELSE %] > >+ No patches > >+ [% END %] > > how about not showing any autolander stuff at all if there aren't any > patches? Good idea. Fixed. dkl

https://bugzilla-stage-tip.mozilla.org updated with latest code. Marc, Lukas, please let me know if you are having any difficulties so we can get them fixed up. dkl

It's working well, just had some auth issues -- it turns out auth info needs to go within the params:{...} portion within the POST body, rather than at the top-level. Having figured all that out, we're good to go, so thanks very much! A side issue though is that the staging Bugzilla has a snapshot from when my mjessome@mozilla.com account had been disabled and prior to my move to my @gmail.com account. Because of this, I'm unable to see the interface for AutoLand on the bugs. Is there anything that we can do about this, should I be filing a bug somewhere for it?

Would it be possible to get another field added in there beside or below the "Branches" field? This new field should allow a user to specify Try Syntax. The data should be made available through the WebService in the same manner as the branch is. There is no need for field verification, but it would be awesome if the field could have a default value of "-b do -p all -u none -t none", sans quotation marks.

Comment on attachment 597966 [details] [diff] [review] Patch to add AutoLand extension to BMO (v5) i get warnings when updating bugs where branches is empty: process_bug.cgi: Use of uninitialized value $old_branches in string ne at ./extensions/TryAutoLand/Extension.pm line 161.

Updated patch to add new Try Syntax field. 1. Try Syntax cannot be blank if Branches has a value 2. Try Syntax has a default value. 3. Branches cannot be blank if one or more attachments are checked. 4. Other cleanup and optimizations. I have pushed the latest version to https://bugzilla-stage-tip.mozilla.org for your testing enjoyment. dkl

Comment on attachment 600493 [details] [diff] [review] Patch to add AutoLand extension to BMO (v6) you have to use bz->add_column to add the column. i've disabled the extension on bugzilla-stage-tip as this change broke it: Unknown column 'try_syntax' in 'field list' [for Statement "SELECT branches, try_syntax FROM autoland_branches WHERE bug_id = ?"]

Updated patch to add code to add column for try_syntax on bugzilla-stage-tip.mozilla.org. Also applied on the server and re-enabled the extension. dkl

dkl, Really liking this, thanks for the quick work with the try syntax! We are having an infra-sec review soon, and lsblakk and I have been discussing some things that may come up. For the moment this is simply us speculating a situation that may come up so we have a few questions about it, although we are not requesting the implementation of any of it at this time. Since we are landing to branches from the bugzilla interface, we are thinking that infrasec may want some sort of authentication layer when flagging a landing. Would the addition of LDAP authentication to interface be possible? As an example, if autoland changes are made, and the save button is pressed, an LDAP Auth box prompts the user. If some sort of authentication like this would be a possibility, could we get a guesstimate as to how long it might take to implement? Essentially, we are thinking that if we are reaching end of Q1, and this is requested during secreview, would it be something quick & easy for you guys to do for us.

> would it be something quick & easy for you guys to do for us. no, it wouldn't be quick or easy :( we're talking about a significant amount of work, and to be honest i don't think adding another authentication prompt to bugzilla is the right thing to do. what we need is linkage between bmo accounts and ldap. signing into your bmo account is strong enough authentication for core security issues, we don't need to burden the user and bugzilla with another layer of authentication. on solution is for LDAP to be extended to include the user's BMO login. another possible solution, should infrasec require ldap auth, would be to add a staging area to autoland, where patches are held until the user logs into autoland (with ldap credentials of course) and approves the run.

Comment on attachment 601038 [details] [diff] [review] Patch to add AutoLand extension to BMO (v7) Review of attachment 601038 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/TryAutoLand/Extension.pm @@ +95,5 @@ > + if (!$dbh->bz_column_info('autoland_branches', 'try_syntax')) { > + $dbh->bz_add_column('autoland_branches', 'try_syntax', { > + TYPE => 'VARCHAR(255)', > + NOTNULL => 1, > + DEFAULT => "''", that's an unusual default; why not use an empty string? the add_column definition should match the abstract_schema definition, so add the default to the abstract. @@ +109,5 @@ > +} > + > +sub _autoland_try_syntax { > + my $self = shift; > + return $self->{'autoland_try_syntax'} if exists $self->{'autoland_try_syntax'}; it would be cleaner to move this check into _preload_bug_data() @@ +122,5 @@ > + WHERE bug_id = ?", { Slice => {} }, $self->id); > + if ($result) { > + $self->{'autoland_branches'} = $result->{'branches'} || ''; > + $self->{'autoland_try_syntax'} = $result->{'try_syntax'} || ''; > + } even if there were no rows matched, you should still populate autoland_branches and autoland_try_syntax with empty strings to avoid requerying the database. @@ +216,5 @@ > + my $new_try_syntax = $params->{'autoland_try_syntax'}; > + > + my $set_attachments = ref $params->{'autoland_attachments'} > + ? $params->{'autoland_attachments'} > + : [ $params->{'autoland_attachments'} ]; if you don't check any patches, autoland_attachments is an empty string, which you're converting into an array_ref with a single element. this triggers "You cannot check one or more patches for AutoLanding and have an empty Branches value." when updating a bug without any patches selected.

(In reply to Byron Jones ‹:glob› from comment #27) > > would it be something quick & easy for you guys to do for us. > > no, it wouldn't be quick or easy :( > Good thing that we know in case this comes up! Thanks

(In reply to Byron Jones ‹:glob› from comment #28) > ::: extensions/TryAutoLand/Extension.pm > @@ +95,5 @@ > > + if (!$dbh->bz_column_info('autoland_branches', 'try_syntax')) { > > + $dbh->bz_add_column('autoland_branches', 'try_syntax', { > > + TYPE => 'VARCHAR(255)', > > + NOTNULL => 1, > > + DEFAULT => "''", > > that's an unusual default; why not use an empty string? > > the add_column definition should match the abstract_schema definition, so > add the default to the abstract. You have to specify empty string that way for the default. At least that is how it is done with in Bugzilla/DB/Schema.pm in other places. MySQL requires single quotes. > > +sub _autoland_try_syntax { > > + my $self = shift; > > + return $self->{'autoland_try_syntax'} if exists $self->{'autoland_try_syntax'}; > > it would be cleaner to move this check into _preload_bug_data() I would rather preload_bug_data to simply load bug data into the proper places and leave the checks for the calling code. > > + if ($result) { > > + $self->{'autoland_branches'} = $result->{'branches'} || ''; > > + $self->{'autoland_try_syntax'} = $result->{'try_syntax'} || ''; > > + } > > even if there were no rows matched, you should still populate > autoland_branches and autoland_try_syntax with empty strings to avoid > requerying the database. Fixed. > @@ +216,5 @@ > > + my $new_try_syntax = $params->{'autoland_try_syntax'}; > > + > > + my $set_attachments = ref $params->{'autoland_attachments'} > > + ? $params->{'autoland_attachments'} > > + : [ $params->{'autoland_attachments'} ]; > > if you don't check any patches, autoland_attachments is an empty string, > which you're converting into an array_ref with a single element. > > this triggers "You cannot check one or more patches for AutoLanding and have > an empty Branches value." when updating a bug without any patches selected. Good catch. Fixed in next iteration. dkl

New patch to fix uninitialized value error.

Comment on attachment 601880 [details] [diff] [review] Patch to add AutoLand extension to BMO (v9) r=glob \o/

(In reply to Byron Jones ‹:glob› from comment #33) > Comment on attachment 601880 [details] [diff] [review] > Patch to add AutoLand extension to BMO (v9) > > r=glob \o/ Thanks glob. I will go ahead and get the security review bug created so we can at least get that going. Marc, Lukas, is that OK? Also keep me up-to-date on how your testing is going and if you find any problems we can get them taken care of quickly. dkl

(In reply to David Lawrence [:dkl] from comment #34) > (In reply to Byron Jones ‹:glob› from comment #33) > > Comment on attachment 601880 [details] [diff] [review] > > Patch to add AutoLand extension to BMO (v9) > > > > r=glob \o/ > > Thanks glob. I will go ahead and get the security review bug created so we > can at least get that going. Marc, Lukas, is that OK? Also keep me > up-to-date on how your testing is going > and if you find any problems we can get them taken care of quickly. > > dkl David, we have a secreview for Autoland scheduled for March 15th at 10am so if you are available to attend that we could lump this all together?

(In reply to Lukas Blakk [:lsblakk] from comment #35) > (In reply to David Lawrence [:dkl] from comment #34) > > (In reply to Byron Jones ‹:glob› from comment #33) > > > Comment on attachment 601880 [details] [diff] [review] > > > Patch to add AutoLand extension to BMO (v9) > > > > > > r=glob \o/ > > > > Thanks glob. I will go ahead and get the security review bug created so we > > can at least get that going. Marc, Lukas, is that OK? Also keep me > > up-to-date on how your testing is going > > and if you find any problems we can get them taken care of quickly. > > > > dkl > > David, we have a secreview for Autoland scheduled for March 15th at 10am so > if you are available to attend that we could lump this all together? Sure if they will do that that would be great. I have created the bug report for the sec review as required (bug 732058) and will comment there to let them know. dkl

A question comes up for me in the thoughts of efficiency. As I'm using this, I've noticed that on every query using GetBugs() I'm getting all bugs with ANY autoland information (whether it is 'waiting', 'success', 'failure' or 'running'). Since jobs will likely be flagged quite often in production, I can imagine that this will cause a significant extra amount of data and load on the BMO end, as well as on ours. Thoughts? Also, currently when querying bugzilla-stage-tip, I am not getting any try syntax field in the returned data. Is this already enabled? Thanks

Changes: 1. Fixed bug where try_syntax was not being returned by TryAutoLand.getBugs properly. 2. Allow param 'status' to be passed to TryAutoLand.getBugs to filter down the results returned by current status. 'status' can be either a single value or a list of values. dkl

Hey all, was chatting with mcoates yesterday at the SF cantina and trying to feel out for any potential security issues that might come up during review. The potential risk seems to be with the web service part - is it being served only over SSL? We'll want that as well as for our scripts that interact with it to be sure there is a valid SSL connection at all times. The risk here is that without guaranteed SSL, someone could possibly inject the username of someone with higher privs and get a push to branch where the real user might not have had permission to do so.

(In reply to Lukas Blakk [:lsblakk] from comment #39) > Hey all, was chatting with mcoates yesterday at the SF cantina and trying to > feel out for any potential security issues that might come up during review. > The potential risk seems to be with the web service part - is it being > served only over SSL? We'll want that as well as for our scripts that > interact with it to be sure there is a valid SSL connection at all times. > The risk here is that without guaranteed SSL, someone could possibly inject > the username of someone with higher privs and get a push to branch where the > real user might not have had permission to do so. As it is just a normal webservice exported by BMO itself it has the same SSL support that BMO does. Clients hitting BMO are always served SSL content even if the client uses http:// using a redirect at the proxy level. So in this case we should be OK. dkl

Comment on attachment 602514 [details] [diff] [review] Patch to add AutoLand extension to BMO (v10) Review of attachment 602514 [details] [diff] [review]: ----------------------------------------------------------------- r=glob both issues are minor and can be fixed in a later revision, or prior to committing. ::: Bugzilla/WebService/Server.pm @@ +26,4 @@ > sub handle_login { > my ($self, $class, $method, $full_method) = @_; > eval "require $class"; > + print STDERR $@ if $@; use warn instead of printing directly to STDERR, so arecibo can catch and report. ::: extensions/TryAutoLand/lib/WebService.pm @@ +37,5 @@ > + ThrowUserError("auth_failure", { action => "access", > + object => "autoland_attachments" }); > + } > + > + use Data::Dumper; i suspect you left this debugging code in.

Thanks glob. Fixed in this latest patch. Moving forward r+ as well. dkl

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bmo/4.0/ modified Bugzilla/WebService/Server.pm added extensions/TryAutoLand added extensions/TryAutoLand/Config.pm added extensions/TryAutoLand/Extension.pm added extensions/TryAutoLand/bin added extensions/TryAutoLand/lib added extensions/TryAutoLand/template added extensions/TryAutoLand/web added extensions/TryAutoLand/bin/TryAutoLand.getBugs.pl added extensions/TryAutoLand/bin/TryAutoLand.updateStatus.pl added extensions/TryAutoLand/bin/TryAutoLand.updateStatus_json.pl added extensions/TryAutoLand/lib/Constants.pm added extensions/TryAutoLand/lib/WebService.pm added extensions/TryAutoLand/template/en added extensions/TryAutoLand/template/en/default added extensions/TryAutoLand/template/en/default/hook added extensions/TryAutoLand/template/en/default/hook/bug added extensions/TryAutoLand/template/en/default/hook/global added extensions/TryAutoLand/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl added extensions/TryAutoLand/template/en/default/hook/bug/field-help-end.none.tmpl added extensions/TryAutoLand/template/en/default/hook/bug/show-header-end.html.tmpl added extensions/TryAutoLand/template/en/default/hook/global/user-error-auth_failure_object.html.tmpl added extensions/TryAutoLand/template/en/default/hook/global/user-error-errors.html.tmpl added extensions/TryAutoLand/web/style.css Committed revision 8108.

Closing. Please file new bugs for any problems. dkl