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.