Bug 218441 - Conditionally disable the patch validation step
Summary: Conditionally disable the patch validation step
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-02 05:42 PST by Angelos Oikonomopoulos
Modified: 2020-11-05 04:20 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.62 KB, patch)
2020-11-02 05:43 PST, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (1.60 KB, patch)
2020-11-04 08:37 PST, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Angelos Oikonomopoulos 2020-11-02 05:42:18 PST
Conditionally disable the patch validation step
Comment 1 Angelos Oikonomopoulos 2020-11-02 05:43:51 PST
Created attachment 412908 [details]
Patch
Comment 2 Aakash Jain 2020-11-02 08:43:49 PST
Comment on attachment 412908 [details]
Patch

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

> Tools/ChangeLog:11
> +        Note that the skip_validation parameter cannot be supplied remotely, so

This build property can be supplied while using 'buildbot try' command (which ews-app uses). So, this note doesn't seems accurate.

> Tools/CISupport/ews-build/steps.py:661
> +        return int(self.getProperty('skip_validation', 0)) == 0

should we make this property boolean instead of 0/1? Something like:

return not self.getProperty('skip_validation', False)
Comment 3 Angelos Oikonomopoulos 2020-11-04 08:07:20 PST
(In reply to Aakash Jain from comment #2)
> Comment on attachment 412908 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412908&action=review
> 
> > Tools/ChangeLog:11
> > +        Note that the skip_validation parameter cannot be supplied remotely, so
> 
> This build property can be supplied while using 'buildbot try' command
> (which ews-app uses). So, this note doesn't seems accurate.

My bad. 'buildbot try' can of course connect to a remote server instance. What I meant to say is that, AFAIU, Try_Userpass uses a separate set of auth credentials, so that one can only set skip_validation by going through the ews-app.

If that's not the case, do you have a suggestion for good way to skip patch validation for testing setups?

> > Tools/CISupport/ews-build/steps.py:661
> > +        return int(self.getProperty('skip_validation', 0)) == 0
> 
> should we make this property boolean instead of 0/1? Something like:
> 
> return not self.getProperty('skip_validation', False)

I actually selected this because it's probably easier for the user to figure out how to specify a boolean property to the buildbot cli client but, as it's unlikely this would be used directly by someone, using a bool seems like an improvement. I'll submit an updated patch.

Thanks!
Comment 4 Aakash Jain 2020-11-04 08:13:14 PST
(In reply to Angelos Oikonomopoulos from comment #3)
> (In reply to Aakash Jain from comment #2)
> > Comment on attachment 412908 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=412908&action=review
> > 
> > > Tools/ChangeLog:11
> > > +        Note that the skip_validation parameter cannot be supplied remotely, so
> > 
> > This build property can be supplied while using 'buildbot try' command
> > (which ews-app uses). So, this note doesn't seems accurate.
> 
> My bad. 'buildbot try' can of course connect to a remote server instance.
> What I meant to say is that, AFAIU, Try_Userpass uses a separate set of auth
> credentials, so that one can only set skip_validation by going through the
> ews-app.
> 
> If that's not the case, do you have a suggestion for good way to skip patch validation for testing setups?
My comment was only about the Note in ChangeLog, the logic looks fine.
Comment 5 Angelos Oikonomopoulos 2020-11-04 08:37:00 PST
Created attachment 413167 [details]
Patch
Comment 6 Angelos Oikonomopoulos 2020-11-04 08:38:34 PST
> > My bad. 'buildbot try' can of course connect to a remote server instance.
> > What I meant to say is that, AFAIU, Try_Userpass uses a separate set of auth
> > credentials, so that one can only set skip_validation by going through the
> > ews-app.
> > 
> > If that's not the case, do you have a suggestion for good way to skip patch validation for testing setups?
> My comment was only about the Note in ChangeLog, the logic looks fine.

OK great. Hopefully reads better in the updated patch.
Comment 7 EWS 2020-11-04 14:27:06 PST
Committed r269391: <https://trac.webkit.org/changeset/269391>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413167 [details].
Comment 8 Radar WebKit Bug Importer 2020-11-04 14:28:23 PST
<rdar://problem/71051939>
Comment 9 Aakash Jain 2020-11-05 04:20:24 PST
would be nice to add a unit-test as well for this.