Bug 206534

<rdar://problem/58759731>

Created attachment 388728 [details]
WIP

Created attachment 388996 [details]
Patch

Comment on attachment 388996 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-app/ews/fetcher.py:65
> +        self.send_patches_to_buildbot(patches_to_send)

Why weren't we doing this before? This seems out of the scope of this patch.

> Tools/BuildSlaveSupport/ews-app/ews/fetcher.py:71
> +        Patch.save_patches(patch_ids_commit_queue)

What does saving a patch do?

> Tools/BuildSlaveSupport/ews-app/ews/fetcher.py:73
> +        _log.info('{} cq+ patches, {} patches need to be sent to commit queue: {}'.format(len(patch_ids_commit_queue), len(patches_to_send), patches_to_send))

We're printing the patches? That seems wrong, unless patches_to_send is actually patch_ids_to_send

> Tools/BuildSlaveSupport/ews-app/ews/fetcher.py:109
> +                    Patch.set_sent_to_commit_queue(patch_id, False)

Maybe this is better achieved by setting a 'send_patch' function at the beginning of this function so we don't have to duplicate these if statements

> Tools/BuildSlaveSupport/ews-app/ews/common/buildbot.py:44
> +    def send_patch_to_buildbot(cls, patch_path, send_to_commit_queue=False, properties=[]):

Small nit: setting an argument to a function to a list like this is bad because it creates a global list. We usually want to do:

def send_patch_to_buildbot(cls, patch_path, send_to_commit_queue=False, properties=None):
    properties = properties if properties else []

Not part of the patch, though, so feel free to ignore it.

> Tools/BuildSlaveSupport/ews-app/ews/common/buildbot.py:-56
> -        _log.debug('Executing command: {}'.format(command))

Why did we drop this logging message?

Comment on attachment 388996 [details]
Patch

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

>> Tools/BuildSlaveSupport/ews-app/ews/fetcher.py:65
>> +        self.send_patches_to_buildbot(patches_to_send)
> 
> Why weren't we doing this before? This seems out of the scope of this patch.

The diff might look visually confusing. We were doing this before as well, I just moved that code to the function send_patches_to_buildbot()

>> Tools/BuildSlaveSupport/ews-app/ews/fetcher.py:71
>> +        Patch.save_patches(patch_ids_commit_queue)
> 
> What does saving a patch do?

Save it in the database table.

>> Tools/BuildSlaveSupport/ews-app/ews/fetcher.py:73
>> +        _log.info('{} cq+ patches, {} patches need to be sent to commit queue: {}'.format(len(patch_ids_commit_queue), len(patches_to_send), patches_to_send))
> 
> We're printing the patches? That seems wrong, unless patches_to_send is actually patch_ids_to_send

Yeah, these are just patch ids.

>> Tools/BuildSlaveSupport/ews-app/ews/fetcher.py:109
>> +                    Patch.set_sent_to_commit_queue(patch_id, False)
> 
> Maybe this is better achieved by setting a 'send_patch' function at the beginning of this function so we don't have to duplicate these if statements

Done in updated patch.

>> Tools/BuildSlaveSupport/ews-app/ews/common/buildbot.py:44
>> +    def send_patch_to_buildbot(cls, patch_path, send_to_commit_queue=False, properties=[]):
> 
> Small nit: setting an argument to a function to a list like this is bad because it creates a global list. We usually want to do:
> 
> def send_patch_to_buildbot(cls, patch_path, send_to_commit_queue=False, properties=None):
>     properties = properties if properties else []
> 
> Not part of the patch, though, so feel free to ignore it.

Done in updated patch.

>> Tools/BuildSlaveSupport/ews-app/ews/common/buildbot.py:-56
>> -        _log.debug('Executing command: {}'.format(command))
> 
> Why did we drop this logging message?

because the command might contain sensitive data like buildbot try password.

Created attachment 389043 [details]
Patch

Comment on attachment 389043 [details]
Patch

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

r+, just take a look at the removed return statement

> Tools/BuildSlaveSupport/ews-app/ews/fetcher.py:-86
> -        return patch_ids

Any reason we removed this return statement?

(In reply to Jonathan Bedard from comment #7)
> Any reason we removed this return statement?
Since the return value was never used.

Comment on attachment 389043 [details]
Patch

Clearing flags on attachment: 389043

Committed r255269: <https://trac.webkit.org/changeset/255269>

All reviewed patches have been landed.  Closing bug.