Bug 218441 – Conditionally disable the patch validation step

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Angelos Oikonomopoulos]

Conditionally disable the patch validation step

[Angelos Oikonomopoulos]

Created attachment 412908 [details]
Patch

[Aakash Jain]

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]

(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]

(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]

Created attachment 413167 [details]
Patch

[Angelos Oikonomopoulos]

> > 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]

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]

<rdar://problem/71051939>

[Aakash Jain]

would be nice to add a unit-test as well for this.