[webkit-reviews] review denied: [Bug 123559] build.webkit.org should have a clean build button : [Attachment 215622] Reverted errornous changes to Authz

[bugzilla-daemon at webkit.org]

Csaba Osztrogonac <ossy at webkit.org> has denied Ryosuke Niwa
<rniwa at webkit.org>'s request for review:
Bug 123559: build.webkit.org should have a clean build button
https://bugs.webkit.org/show_bug.cgi?id=123559

Attachment 215622: Reverted errornous changes to Authz
https://bugs.webkit.org/attachment.cgi?id=215622&action=review

------- Additional Comments from Csaba Osztrogonac <ossy at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=215622&action=review


In general I like the idea to have a clean build feature 
on build.webkit.org, I have only some minor remarks.

> Tools/ChangeLog:16
> +	   (CleanBuildIfScheduled.start): Added. Don't do a clean build if if
this build wasn't schecueld by

typo: schecueld

> Tools/BuildSlaveSupport/build.webkit.org-config/make-passwords-json.py:12
> +#!/usr/bin/python
> +
> +import json
> +
> +with open('config.json', 'r') as config_file:
> +    config = json.load(config_file)
> +    passwords = {}
> +    for slave in config['slaves']:
> +	   passwords[slave['name']] = 'a'
> +
> +with open('passwords.json', 'w') as passwords_file:
> +    passwords_file.write(json.dumps(passwords))

It is a good idea to have this script for testing. ;)

But it would be great if we can avoid code duplication here. I mean we 
can move _create_mock_passwords_dict from mastercfg_unittest.py to this file.
And then mastercfg_unittest.py can simple import and use it. And we can
generate
the password.json in this file in an "if __name__ == '__main__':" block.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:922
> +    clean_scheduler = ForceScheduler(name="clean",
> +	   builderNames=[str(builder['name']) for builder in
config['builders']],
> +	   reason=FixedParameter(name="reason", default="The 'Clean Build'
button was pressed."),
> +
> +	   revision=StringParameter(name="revision", default="",
regex=re.compile(r'^(\d*)$')),
> +
> +	   branch=FixedParameter(name="branch"),
> +	   repository=FixedParameter(name="repository"),
> +	   project=FixedParameter(name="project"),
> +	   properties=[])
> +    c['schedulers'].append(clean_scheduler)

I prefer adding a new BooleanParameter to the existing ForceScheduler
with clean name and False default value.