Bug 201733 – block-spammers should obtain credentials the same way as webkit-patch

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Kilzer (:ddkilzer)]

The block-spammers script should obtain credentials the same way as webkit-patch.

[David Kilzer (:ddkilzer)]

Created attachment 378669 [details]
Patch v1

[Alexey Proskuryakov]

Comment on attachment 378669 [details]
Patch v1

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

> Tools/Scripts/block-spammers:34
> +from webkitpy.common.net.credentials import Credentials

Does this work when you don't have autoimported modules in place yet?

[David Kilzer (:ddkilzer)]

Created attachment 378670 [details]
Patch v2

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #3)
> Created attachment 378670 [details]
> Patch v2

Changed double quotes to single quotes in one place.

[David Kilzer (:ddkilzer)]

(In reply to Alexey Proskuryakov from comment #2)
> Comment on attachment 378669 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378669&action=review
> 
> > Tools/Scripts/block-spammers:34
> > +from webkitpy.common.net.credentials import Credentials
> 
> Does this work when you don't have autoimported modules in place yet?

No clue what that is, why I'd want it, or how to test it.

If you're set up that way, could you take two minutes to apply the patch and test it?

Since this uses the exact same code path as webkit-patch, I presume that tool would also be broken if that configuration isn't supported.

[Alexey Proskuryakov]

What happens if you "rm -rf Tools/Scripts/webkitpy/thirdparty/autoinstalled", and run this script? Webkitpy has installed those modules for you the first time you used it.

The code path in webkitpy is not exactly the same, and autoimport setup is subtle.

[Alexey Proskuryakov]

(what should happen is that everything works anyway, and that the keyring module re-appears in autoinstalled)

[David Kilzer (:ddkilzer)]

(In reply to Alexey Proskuryakov from comment #6)
> What happens if you "rm -rf
> Tools/Scripts/webkitpy/thirdparty/autoinstalled", and run this script?
> Webkitpy has installed those modules for you the first time you used it.
> 
> The code path in webkitpy is not exactly the same, and autoimport setup is
> subtle.

(In reply to Alexey Proskuryakov from comment #7)
> (what should happen is that everything works anyway, and that the keyring
> module re-appears in autoinstalled)

$ mv Tools/Scripts/webkitpy/thirdparty/autoinstalled Tools/Scripts/webkitpy/thirdparty/autoinstalled.BAK
$ ./Tools/Scripts/block-spammers [e-mail]
No handlers could be found for logger "webkitpy.common.net.credentials"
Fetching account activity...
[...]

Seems to work in the case where the credentials are already in the keychain.

[Alexey Proskuryakov]

> No handlers could be found for logger "webkitpy.common.net.credentials"

This looks like a regression from this patch that this line is logged.

[Alexey Proskuryakov]

Not trying to be difficult, but I think that the new console spew needs to be fixed before landing.

[David Kilzer (:ddkilzer)]

(In reply to Alexey Proskuryakov from comment #10)
> Not trying to be difficult, but I think that the new console spew needs to
> be fixed before landing.

No worries. I had to leave work early, then I had figure out how to fix the logging warning, then I decided to refactor a bit of credentials.py.

[David Kilzer (:ddkilzer)]

Created attachment 378710 [details]
Patch v3

[David Kilzer (:ddkilzer)]

Created attachment 378712 [details]
Patch v4

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #12)
> Created attachment 378710 [details]
> Patch v3

This fixed the new warning and refactored credentials.py.

(In reply to David Kilzer (:ddkilzer) from comment #13)
> Created attachment 378712 [details]
> Patch v4

This changed the logging level to info, which matches webkit-patch, so now they behave the same way (including the same console output) when reading credentials from the keychain.

[WebKit Commit Bot]

Comment on attachment 378712 [details]
Patch v4

Clearing flags on attachment: 378712

Committed r249884: <https://trac.webkit.org/changeset/249884>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55378246>