Last modified: 2014-05-06 15:35:27 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T38228, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 36228 - Add specific reviewer in Gerrit on SQL changes
Add specific reviewer in Gerrit on SQL changes
Status: RESOLVED WONTFIX
Product: Wikimedia
Classification: Unclassified
Git/Gerrit (Other open bugs)
unspecified
All All
: Normal minor (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-25 07:11 UTC by Antoine "hashar" Musso (WMF)
Modified: 2014-05-06 15:35 UTC (History)
12 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Antoine "hashar" Musso (WMF) 2012-04-25 07:11:07 UTC
From wikitech-l by Rob Lanphier about DB changes
http://permalink.gmane.org/gmane.science.linguistics.wikipedia.technical/60967

> 3.  For anything that involves a schema change to the production dbs,
> make sure Asher Feldman (<yum I really love spam>) is on the reviewer
> list.  He's already keeping an eye on this stuff the best he can, but
> it's going to be easy for him to miss changes in extensions should
> they happen.

I am pretty sure Jenkins could detect a change is being made on a .sql
file and then add a reviewer using the CLI tool.
Comment 1 Chad H. 2012-04-25 17:08:43 UTC
It could also be done on patchset-created, I don't think we need jenkins for this.
Comment 2 Greg Sabino Mullane 2012-04-25 18:35:15 UTC
If you do this, please add me for (postgres|pg).*\.sql$ if possible please :)
Comment 3 Marcin Cieślak 2012-05-09 11:01:29 UTC
This should actually be done in Gerrit (there is facility for that). Will have a look.
Comment 4 Chad H. 2012-06-18 18:25:35 UTC
Suggestion from upstream was to add to rules.pl (could probably get away with doing it in All-Projects).

Anyone know Prolog?
Comment 5 Antoine "hashar" Musso (WMF) 2012-06-18 19:50:25 UTC
We can use gerrit 'query' to get files changed by a patchset.

$ gerrit query 11899 --patch-sets --files --format=json > ~/JSON
+ ssh -p 29418 hashar@gerrit.wikimedia.org 'gerrit query 11899 --patch-sets --files --format=json'


Gives you a nice JSON structure to parse

            "files":[
                {"file":"/COMMIT_MSG",
                    "type":"ADDED"
                },
                {"file":"additions/php_file.php",
                    "type":"ADDED"
                },
                {"file":"additions/sql_file.php",
                    "type":"ADDED"
                }
            ]

The Gerrit trigger plugin does publish the change number as an environment variable (GERRIT_CHANGE_NUMBER). We could then write a script that would query Gerrit for the list of files that change involve (command above), then based on some logic the script could set a reviewer on the change (using gerrit set-reviewers).

Commands:

 gerrit query $GERRIT_CHANGE_NUMBER --patch-sets --files --format=json 

 gerrit set-reviewers $GERRIT_CHANGE_NUMBER -add someone@example.org
Comment 6 Antoine "hashar" Musso (WMF) 2012-06-19 12:05:45 UTC
(note, I am implicitly discarding implementing such a feature in Gerrit just because nobody knows prolog)
Comment 7 Antoine "hashar" Musso (WMF) 2012-06-28 06:25:11 UTC
I added a very basic ant rule to detect if a SQL file got changed: https://gerrit.wikimedia.org/r/#/c/13169/

Need to wrap it in a Jenkins job called from the MediaWiki-GIT-Fetching job.
Comment 8 Chris McMahon 2012-10-09 21:24:10 UTC
we need this for beta labs in order to update db tables in sync with updated code, ahead of production
Comment 9 Chris McMahon 2012-10-09 21:24:36 UTC
we need this for beta labs in order to update db tables in sync with updated code, ahead of production
Comment 10 Tim Landscheidt 2012-10-09 23:04:08 UTC
(In reply to comment #9)
> we need this for beta labs in order to update db tables in sync with updated
> code, ahead of production

Wouldn't it be better for that purpose to check in the deployment script if there were any SQL changes between $DEPLOYED_SHA1 and $SHA1_TO_DEPLOY instead of keeping books on changes to come?
Comment 11 Antoine "hashar" Musso (WMF) 2012-10-15 20:47:07 UTC
One possibility would be to change update.php so it can be made to report any change made when doing the update. A Jenkins job could then install from master then update the code base to the change version and run update.php.  Whenever update.php change something the Jenkins job could log / warn someone.
Comment 12 Chad H. 2012-10-15 20:51:52 UTC
(In reply to comment #11)
> One possibility would be to change update.php so it can be made to report any
> change made when doing the update. A Jenkins job could then install from master
> then update the code base to the change version and run update.php.  Whenever
> update.php change something the Jenkins job could log / warn someone.

That would be far too late, since it wouldn't notify the person until after it merged. Also it involves hacking up update.php just to support our workflow.

Got to be a better way.
Comment 13 Tim Landscheidt 2012-10-16 15:20:11 UTC
I gather from the comments and the non-replies that not only need someone be added as a reviewer, but the merge itself should only happen if someone from beta/deployment green-lights it (+1 or +2).

So we need to:

- define this group,
- adapt rules.pl.

For the latter, we need:

- someone knowledgable in/willing to learn Prolog,
- rules.pl.

So let's start at the bottom: Where is it?  Grepping over the operations/puppet and operations/gerrit didn't show anything obvious.
Comment 14 Antoine "hashar" Musso (WMF) 2012-10-19 15:39:35 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > One possibility would be to change update.php so it can be made to report any
> > change made when doing the update. A Jenkins job could then install from master
> > then update the code base to the change version and run update.php.  Whenever
> > update.php change something the Jenkins job could log / warn someone.
> 
> That would be far too late, since it wouldn't notify the person until after it
> merged. Also it involves hacking up update.php just to support our workflow.
> 
> Got to be a better way.

Under a jenkins job, the patchset is not merged yet so that should be fine :-]
Comment 15 Antoine "hashar" Musso (WMF) 2012-10-23 21:50:49 UTC
restoring bug summary. This is really about detecting a SQL change, not preventing the change to be merged.
Comment 16 Waldir 2013-02-14 01:07:29 UTC
(In reply to comment #0)
> From wikitech-l by Rob Lanphier about DB changes
> http://permalink.gmane.org/gmane.science.linguistics.wikipedia.technical/
> 60967
> 
> > 3.  For anything that involves a schema change to the production dbs,
> > make sure Asher Feldman (<yum I really love spam>) is on the reviewer
> > list.  He's already keeping an eye on this stuff the best he can, but
> > it's going to be easy for him to miss changes in extensions should
> > they happen.
> 
> I am pretty sure Jenkins could detect a change is being made on a .sql
> file and then add a reviewer using the CLI tool.

(In reply to comment #2)
> If you do this, please add me for (postgres|pg).*\.sql$ if possible please :)

I think this kind of conditional reviewer-adding is already working with [[mw:Git/Reviewers]]. I'm not sure this counts as solving the bug, but it works for me.
Comment 17 Andre Klapper 2013-09-26 14:30:14 UTC
[Assignee was removed, hence also resetting ASSIGNED status]
Comment 18 Antoine "hashar" Musso (WMF) 2014-05-06 15:35:27 UTC
This can be done in Gerrit by watching a project with a specially crafted search query. Simply head to https://gerrit.wikimedia.org/r/#/settings/projects then:

 Project: mediawiki/core
 Search: file:^.*\.sql$

Of courses some nasty SQL queries can be crafted from PHP files so there is nothing that can replace human review.  Abandoning this bug.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links