Move sensitive configuration options into separate files #93
Conversation
| @@ -2,6 +2,12 @@ | |||
| # License, v. 2.0. If a copy of the MPL was not distributed with this | |||
| # file, You can obtain one at http://mozilla.org/MPL/2.0/. | |||
|
|
|||
| try: | |||
| import configparser | |||
| except ImportError: | |||
There was a problem hiding this comment.
Hey @davehunt a small comment here: in my experience it is better to try to import using a version check rather than fallback from an import error, because the latter might make it harder to debug some import errors. For example, if one has a botched installation or someone has mocked configparser somehow and it fails to import, it will fallback to the second import and "hide" the original error (in Python 2 at least, in Python 3 it won't be a big deal). I had this issue in pytest-dev/pytest-mock#68, where import mock was failing because some missing dependencies.
In summary, I suggest using this instead:
if sys.version_info[0] == 2:
import ConfigParser as configparser
else:
import configparser
| try: | ||
| username = config.get('credentials', 'username') | ||
| except (configparser.NoSectionError, configparser.NoOptionError, KeyError): | ||
| username = os.getenv('BROWSERSTACK_USERNAME') | ||
| if not username: | ||
| raise pytest.UsageError('BrowserStack username must be set') |
There was a problem hiding this comment.
It would be nice to mention BROWSERSTACK_USERNAME in this error message.
| except (configparser.NoSectionError, configparser.NoOptionError, KeyError): | ||
| key = os.getenv('BROWSERSTACK_ACCESS_KEY') | ||
| if not key: | ||
| raise pytest.UsageError('BrowserStack access key must be set') |
There was a problem hiding this comment.
Ditto on this error message as well.
|
|
||
| def _username(config): | ||
| username = config.getini('sauce_labs_username') | ||
| def credentials(): |
There was a problem hiding this comment.
Perhaps it would be worth refactoring credentials so it can be reused by the various drivers? The logic seems to be identical across them: read a config file, try to get the credentials from it, fallback to an environment variable, etc.
|
|
||
|
|
||
| def test_invalid_credentials_env(failure, monkeypatch, tmpdir): | ||
| monkeypatch.setattr(os.path, 'expanduser', lambda p: str(tmpdir)) |
There was a problem hiding this comment.
If you refactor credentials() so it can be reused, I suggest then refactoring the tests as well so you can use the same set of tests related to credentials in all the drivers.
I usually do this by having a base class with test methods, and then subclass that base class in each module configuring it (perhaps using class variables) with the specifics for that driver.
|
@nicoddemus thank you so much for the review... your suggestions were spot on! I've addressed the duplication in the cloud drivers, but have left the test duplication for now. I've also introduced a much more helpful custom exception. This refactoring will allow for further improvements, but I'll leave those for another time. If you get a chance, I'd appreciate another review. |
| **1.8.0 (unreleased)** | ||
|
|
||
| * **BREAKING CHANGE:** Moved cloud testing provider credentials into separate | ||
| files for improved security. |
There was a problem hiding this comment.
I think it would be nice to mention the changes in a little more detail, perhaps mentioning #60, and a little example of what users should do to migrate their current configuration.
There was a problem hiding this comment.
Thanks, I've made some updates and rebased/squashed in preparation for a merge and release later today. I really appreciate your time @nicoddemus!
There was a problem hiding this comment.
Absolutely, my pleasure! 😁
Thanks for all the time and effort you put in your excellent plugins as well! 👍
Read credentials for cloud testing services from configuration files in the working or home directory. Fixes pytest-dev#60
Read credentials for cloud testing services from configuration files in the working or home directory. Fixes #60