[webkit-reviews] review denied: [Bug 65200] CSS Regions build bot should archive and upload output files : [Attachment 102108] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 65200: CSS Regions build bot should archive and upload output files
https://bugs.webkit.org/show_bug.cgi?id=65200

Attachment 102108: Patch
https://bugs.webkit.org/attachment.cgi?id=102108&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=102108&action=review


> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:132
> +def determineExtraFeatures(build):

Can this be a member of the UploadBuiltProduct class?

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:134
> +    if (build.hasProperty("features")):

No need for the parentheses here.

I think using an early return would be clearer:

if not build.hasProperty('features'):
    return ''

Ditto for the len(features) check below.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:141
> +

Missing an extra newline here.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:623
>	   if triggers:
>	       self.addStep(ArchiveBuiltProduct)
>	       self.addStep(UploadBuiltProduct)
> -	       self.addStep(trigger.Trigger, schedulerNames=triggers)
> +	       if triggers.count("upload-only") != len(triggers):
> +		   self.addStep(trigger.Trigger, schedulerNames=triggers)

Is it OK to be passing "upload-only", which isn't a real trigger, to the
schedulerNames parameter? Maybe it would be better to have some other way of
specifying in config.json that you want the build uploaded. Then here you could
do "if triggers or thatOtherThing:"