Bug 100563 – garden-o-matic should support gardening a single port and specifying how to deal with overwritten baselines

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

garden-o-matic should support gardening a single port and specifying how to deal with overwritten baselines

[Dirk Pranke]

Created attachment 171014 [details]
Patch

[Dirk Pranke]

Comment on attachment 171014 [details]
Patch

patch isn't quite ready for review yet; the basic idea is implemented but I need to change the way I did things in the UI / JS code, and then update all of the unit tests.

[Ojan Vafai]

Comment on attachment 171014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171014&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/config.js:172
> +        // FIXME: If currentBuilder is set, we want to limit any place that iterates over
> +        // a list of builders to just use the currentBuilder. instead of the full list of 
> +        // builders for the current platform.

Maybe also add a FIXME to add UI to pick a builder?

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 171014 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171014&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/config.js:172
> > +        // FIXME: If currentBuilder is set, we want to limit any place that iterates over
> > +        // a list of builders to just use the currentBuilder. instead of the full list of 
> > +        // builders for the current platform.
> 
> Maybe also add a FIXME to add UI to pick a builder?

Yup, that was my intent.

[Dirk Pranke]

Created attachment 171533 [details]
rebasing

[Dirk Pranke]

Created attachment 171564 [details]
Patch

[Ojan Vafai]

Comment on attachment 171564 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171564&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/config.js:151
> +            if (config.currentBuilder)
> +                return builderName == config.currentBuilder;
> +            

Lets not have this logic in this function. Lets instead have a helper function that is the sole caller of builderApplies and make builderApplies private perhaps.

> Tools/Scripts/webkitpy/tool/commands/gardenomatic.py:46
> +            # FIXME: This assumes that the implementation is the first part of options.platform.

I'm not sure what you mean by "implementation" here.

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:66
> +            arg_string = '?' + '&'.join("%s=%s" % (key, urllib.quote(value)) for (key, value) in args.items())

Can you just use urllib.urlencode(args) or something like that here?

[Dirk Pranke]

Comment on attachment 171564 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171564&action=review

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/config.js:151
>> +            
> 
> Lets not have this logic in this function. Lets instead have a helper function that is the sole caller of builderApplies and make builderApplies private perhaps.

Sounds good.

>> Tools/Scripts/webkitpy/tool/commands/gardenomatic.py:46
>> +            # FIXME: This assumes that the implementation is the first part of options.platform.
> 
> I'm not sure what you mean by "implementation" here.

"implementation" is what I call the "chromium-" or "gtk-" part of the full port name, but it's not a widely used term (in fact, it may not actually show up anywhere in the code, I'm not sure). I'll rework the comment.

>> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:66
>> +            arg_string = '?' + '&'.join("%s=%s" % (key, urllib.quote(value)) for (key, value) in args.items())
> 
> Can you just use urllib.urlencode(args) or something like that here?

Oh, yeah. I'll do that. I'm not that familiar w/ urllib and forgot about that.

[Dirk Pranke]

Created attachment 171697 [details]
update w/ review feedback

[Dirk Pranke]

(In reply to comment #8)
> >> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:66
> >> +            arg_string = '?' + '&'.join("%s=%s" % (key, urllib.quote(value)) for (key, value) in args.items())
> > 
> > Can you just use urllib.urlencode(args) or something like that here?
> 
> Oh, yeah. I'll do that. I'm not that familiar w/ urllib and forgot about that.

Sigh. Turns out urllib.urlencode encodes spaces as plus signs, and the buildbots choke on that. I added a comment to note this.

[Dirk Pranke]

Please take another look?

[Dirk Pranke]

Committed r133060: <http://trac.webkit.org/changeset/133060>