Bug 200146 – results.webkit.org: Support buildbot 0.8 CI links

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

At the moment, the results database only supports buildbot 0.9 and above. We should add a factory for buildbot 0.8 too.

[Jonathan Bedard]

Created attachment 374926 [details]
Patch

[Aakash Jain]

Comment on attachment 374926 [details]
Patch

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

> Tools/resultsdbpy/resultsdbpy/model/ci_context.py:210
> +class OldBuildbotURLFactory(object):

Old is relative. After few years, Buildbot 1 or Buildbot 2 might seem old. Better to name it something like: BuildbotEightURLFactory

In http://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js#L74, we used VERSION_LESS_THAN_09, but that name seems more relevant to flag name rather than class name.

> Tools/resultsdbpy/resultsdbpy/model/ci_context.py:217
> +        if builder_name and not worker_name:

Should we add an assert/log here that should_fetch should always be False here?

> Tools/resultsdbpy/resultsdbpy/model/ci_context.py:219
> +                return self.SCHEME + urllib.parse.quote(f'{self.master}/builders/{builder_name}/builds/{build_number}')

why not just have self.SCHEME also inside urlib.parse.quote to keep the complete url together.

> Tools/resultsdbpy/resultsdbpy/model/ci_context.py:223
> +        return None

what if both builder_name and worker_name is passed? We should atleast return an error message or log an error.

> Tools/resultsdbpy/resultsdbpy/model/ci_context_unittest.py:262
> +        self.assertEqual('https://build.webkit.org/builders/Apple%20Mojave%20Release%20%28Build%29/builds/7170', factory.url(builder_name='Apple Mojave Release (Build)', build_number=7170))

Please add a test case for the scenario when both builder_name and worker_name are specified. Also for the case when none of them is specified.

[Jonathan Bedard]

Comment on attachment 374926 [details]
Patch

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

>> Tools/resultsdbpy/resultsdbpy/model/ci_context.py:217
>> +        if builder_name and not worker_name:
> 
> Should we add an assert/log here that should_fetch should always be False here?

No, because this class needs to be a drop in replacement for BuildbotURLFactory, which needs to talk to the buildbot instance to construct URLs. That's basically what should_fetch is used for, it's value doesn't matter in this class.

>> Tools/resultsdbpy/resultsdbpy/model/ci_context.py:219
>> +                return self.SCHEME + urllib.parse.quote(f'{self.master}/builders/{builder_name}/builds/{build_number}')
> 
> why not just have self.SCHEME also inside urlib.parse.quote to keep the complete url together.

Because urlib.parse.quote will escape :

>> Tools/resultsdbpy/resultsdbpy/model/ci_context.py:223
>> +        return None
> 
> what if both builder_name and worker_name is passed? We should atleast return an error message or log an error.

We really don't want to actually return an error message, the error would get saved in the database as the url. We can't raise an exception because this would prevent the URLs from being saved into the database. We could log an error, but we didn't do that in BuildbotURLFactory (so we'd need to do it there too). The caller is aware that this function could return None (more likely with BuildbotURLFactory than it is with this class anyways, since it will return None if the builder_name/worker_name doesn't exist) so I don't know how helpful logging the error would be. We can add that if you'd like, though.

[Jonathan Bedard]

Created attachment 374967 [details]
Patch

[Aakash Jain]

Comment on attachment 374926 [details]
Patch

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

>>> Tools/resultsdbpy/resultsdbpy/model/ci_context.py:223
>>> +        return None
>> 
>> what if both builder_name and worker_name is passed? We should atleast return an error message or log an error.
> 
> We really don't want to actually return an error message, the error would get saved in the database as the url. We can't raise an exception because this would prevent the URLs from being saved into the database. We could log an error, but we didn't do that in BuildbotURLFactory (so we'd need to do it there too). The caller is aware that this function could return None (more likely with BuildbotURLFactory than it is with this class anyways, since it will return None if the builder_name/worker_name doesn't exist) so I don't know how helpful logging the error would be. We can add that if you'd like, though.

Yeah, let's add a log (in both the classes). Add it, would make it clear in the code as well, that case is currently hidden in the if-else, and reading the code doesn't make it obvious. We can occasionally grep the logs for such error message to find out if this condition ever happened.

[Aakash Jain]

Comment on attachment 374967 [details]
Patch

r+ with comment about adding a log.

[Jonathan Bedard]

Created attachment 374974 [details]
Patch

[Aakash Jain]

Comment on attachment 374974 [details]
Patch

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

> Tools/resultsdbpy/resultsdbpy/model/ci_context.py:225
> +        logging.error('No URL can be generated for the provided combination')

I preferred specific elif, something like:

elif builder_name and worker_name:
    logging.error('Both builder_name and worker_name provided to generate URL, please provide only one.')

better yet, maybe just default to constructing builder_name url, if both builder_name and worker_name are provided.

[Jonathan Bedard]

Created attachment 374978 [details]
Patch

[Aakash Jain]

Comment on attachment 374978 [details]
Patch

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

lgtm

> Tools/resultsdbpy/resultsdbpy/model/ci_context_unittest.py:255
> +            self.assertEqual('https://build.webkit.org/#/builders/1', factory.url(builder_name='Mojave-Release-Builder', worker_name='builder1', should_fetch=True))

Nit: we should also test the case when builder_name is provided but worker_name is not provided.

[Jonathan Bedard]

Comment on attachment 374978 [details]
Patch

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

>> Tools/resultsdbpy/resultsdbpy/model/ci_context_unittest.py:255
>> +            self.assertEqual('https://build.webkit.org/#/builders/1', factory.url(builder_name='Mojave-Release-Builder', worker_name='builder1', should_fetch=True))
> 
> Nit: we should also test the case when builder_name is provided but worker_name is not provided.

We already do, line 235

[Aakash Jain]

> We already do, line 235
ok, wasn't easily visible in the review window. lgtm.

[WebKit Commit Bot]

Comment on attachment 374978 [details]
Patch

Clearing flags on attachment: 374978

Committed r247870: <https://trac.webkit.org/changeset/247870>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53664310>