Teach webkit-patch how to read credentials from the environment
Created attachment 71812 [details] Patch
Created attachment 71813 [details] Patch
Comment on attachment 71813 [details] Patch ok
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?
err, didn't mean to clear abarth's r+
(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.
Created attachment 71835 [details] Fixed Tony's nit.
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.
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)
Created attachment 71916 [details] Patch for landing
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.
Comment on attachment 71916 [details] Patch for landing Clearing flags on attachment: 71916 Committed r70562: <http://trac.webkit.org/changeset/70562>
All reviewed patches have been landed. Closing bug.
Committed r70586: <http://trac.webkit.org/changeset/70586>