Bug 48275 – Teach webkit-patch how to read credentials from the environment

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Teach webkit-patch how to read credentials from the environment

[Eric Seidel (no email)]

Created attachment 71812 [details]
Patch

[Eric Seidel (no email)]

Created attachment 71813 [details]
Patch

[Adam Barth]

Comment on attachment 71813 [details]
Patch

ok

[Tony Chang]

Comment on attachment 71813 [details]
Patch

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

I'm a bit scared of people putting this in an environment variable.  I think it's more common for environment variables to be leaked accidentally (e.g., build-webkit shows your env or crash dumps include your env).

It might be better to read a named file on disk or try to read the config file directly from ~/.subversion/auth.

> WebKitTools/Scripts/webkitpy/common/net/credentials.py:128
> +    def _credentials_from_environment(self):
> +        return (self._read_environ("username"), self._read_environ("password"))

I'm not convinced _read_environ and _environ_prefix buys you much over WEBKIT_BUGZILLA_USERNAME and WEBKIT_BUGZILLA_PASSWORD.  It does make grepping harder.

> WebKitTools/Scripts/webkitpy/common/net/credentials.py:144
> +            (username, password) = self._credentials_from_git()
>          if not username or not password:
>              (username, password) = self._credentials_from_keychain(username)

Nit: don't need () on the left side of =

> WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py:132
>          os.rmdir(temp_dir_path)

Do we leak this dir if the test fails?

> WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py:153
> +        # FIXME: Seems 'with' would work better than a try/finally here.

How would 'with' ensure that os.rmdir is called?

[Tony Chang]

err, didn't mean to clear abarth's r+

[Eric Seidel (no email)]

(In reply to comment #4)
> (From update of attachment 71813 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71813&action=review
> 
> I'm a bit scared of people putting this in an environment variable.  I think it's more common for environment variables to be leaked accidentally (e.g., build-webkit shows your env or crash dumps include your env).

Crash dumps (at least on mac) do not contain your environment.  It would be concerning if they did. :)

I see your concern, but I think for the current design this is simplest.

> It might be better to read a named file on disk or try to read the config file directly from ~/.subversion/auth.

well, these are bugzilla credentials, which are different than your svn credentials.

> > WebKitTools/Scripts/webkitpy/common/net/credentials.py:128
> > +    def _credentials_from_environment(self):
> > +        return (self._read_environ("username"), self._read_environ("password"))
> 
> I'm not convinced _read_environ and _environ_prefix buys you much over WEBKIT_BUGZILLA_USERNAME and WEBKIT_BUGZILLA_PASSWORD.  It does make grepping harder.

I can see that.  The goal of all this code in webkitpy was not to be unecessarily webkit specific.  That goal may be a bad idea, but it's what I've been going with for now.

> > WebKitTools/Scripts/webkitpy/common/net/credentials.py:144
> > +            (username, password) = self._credentials_from_git()
> >          if not username or not password:
> >              (username, password) = self._credentials_from_keychain(username)
> 
> Nit: don't need () on the left side of =

Will fix.

> > WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py:132
> >          os.rmdir(temp_dir_path)
> 
> Do we leak this dir if the test fails?

Possibly.  Not sure.  I didn't edit the test.

> > WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py:153
> > +        # FIXME: Seems 'with' would work better than a try/finally here.
> 
> How would 'with' ensure that os.rmdir is called?

Well, one can certainly use 'with' with TemporaryFile, maybe not with a temporary directory.  the __entered__ and __exited__ do the right thing.

[Eric Seidel (no email)]

Created attachment 71835 [details]
Fixed Tony's nit.

[Tony Chang]

Comment on attachment 71835 [details]
Fixed Tony's nit.

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

> WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py:132
>          os.rmdir(temp_dir_path)

While you're here, can you put this in a try/finally to make sure that this gets cleaned up even on failure?

> WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py:153
> +        # FIXME: Seems 'with' would work better than a try/finally here.

Can you remove this comment?  The objects in tempfile that depend on scope seem to be specific to files, not directories.  I don't think 'with' would work here.  Alternately, if there is a way to get 'with' to delete the directory, even on failure, then go ahead and make the change.

[Eric Seidel (no email)]

I just made the class I wanted all along:

class _TemporaryDirectory(object):
    def __init__(self, **kwargs):
        self._kwargs = kwargs
        self._directory_path = None

    def __enter__(self):
        self._directory_path = tempfile.mkdtemp(**self._kwargs)
        return self._directory_path

    def __exit__(self, type, value, traceback):
        os.rmdir(self._directory_path)

[Eric Seidel (no email)]

Created attachment 71916 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 71916 [details]:

fast/dom/onerror-img.html

Please file bugs against the tests.  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 71916 [details]
Patch for landing

Clearing flags on attachment: 71916

Committed r70562: <http://trac.webkit.org/changeset/70562>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Committed r70586: <http://trac.webkit.org/changeset/70586>