RESOLVED FIXED 218441
Conditionally disable the patch validation step
https://bugs.webkit.org/show_bug.cgi?id=218441
Summary Conditionally disable the patch validation step
Angelos Oikonomopoulos
Reported 2020-11-02 05:42:18 PST
Conditionally disable the patch validation step
Attachments
Patch (1.62 KB, patch)
2020-11-02 05:43 PST, Angelos Oikonomopoulos
no flags
Patch (1.60 KB, patch)
2020-11-04 08:37 PST, Angelos Oikonomopoulos
no flags
Angelos Oikonomopoulos
Comment 1 2020-11-02 05:43:51 PST
Aakash Jain
Comment 2 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)
Angelos Oikonomopoulos
Comment 3 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!
Aakash Jain
Comment 4 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.
Angelos Oikonomopoulos
Comment 5 2020-11-04 08:37:00 PST
Angelos Oikonomopoulos
Comment 6 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.
EWS
Comment 7 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].
Radar WebKit Bug Importer
Comment 8 2020-11-04 14:28:23 PST
Aakash Jain
Comment 9 2020-11-05 04:20:24 PST
would be nice to add a unit-test as well for this.
Note You need to log in before you can comment on or make changes to this bug.