Config refactor #96
Config refactor #96
Conversation
Why these changes are being introduced: * Adding a Config class simplifies the loading of config values from env variables and allows for the removal of the click options How this addresses that need: * Add Config class with load_config_variables method and corresponding unit tests * Refactor deposit and listen CLI commands with new Config class * Remove all click options from CLI commands and update corresponding CLI tests * Update _aws_credentials fixtures to _test_env fixture with all required env variables Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-898
There was a problem hiding this comment.
Looking really good and is functional as-is!
I left some thoughts and suggestions, that might help colocate the list of required env vars and remove the need for attr-defined linting skips. Understanding this PR to the first of a standardization of configurations / env vars, so just thinking broadly about patterns, even those that may not immediately benefit this project.
tests/conftest.py
Outdated
| def _test_env(): | ||
| os.environ["AWS_ACCESS_KEY_ID"] = "testing" | ||
| os.environ["AWS_DEFAULT_REGION"] = "us-east-1" | ||
| os.environ["AWS_SECRET_ACCESS_KEY"] = "testing" # noqa: S105 | ||
| os.environ["WORKSPACE"] = "test" | ||
| os.environ["LOG_LEVEL"] = "INFO" | ||
| os.environ["DOI_TABLE"] = "wiley-test" | ||
| os.environ["METADATA_URL"] = "http://example.com/works/" | ||
| os.environ["CONTENT_URL"] = "http://example.com/doi/" | ||
| os.environ["BUCKET"] = "awd" | ||
| os.environ["SQS_BASE_URL"] = "https://queue.amazonaws.com/123456789012/" | ||
| os.environ["SQS_INPUT_QUEUE"] = "mock-input-queue" | ||
| os.environ["SQS_OUTPUT_QUEUE"] = "mock-output-queue" | ||
| os.environ["COLLECTION_HANDLE"] = "123.4/5678" | ||
| os.environ["LOG_SOURCE_EMAIL"] = "noreply@example.com" | ||
| os.environ["LOG_RECIPIENT_EMAIL"] = "mock@mock.mock" | ||
| os.environ["RETRY_THRESHOLD"] = "10" | ||
| os.environ["SENTRY_DSN"] = "None" |
There was a problem hiding this comment.
Genuinely curious others' thoughts, but we could start to standardize around using monkeypatch (https://docs.pytest.org/en/7.1.x/how-to/monkeypatch.html) for environment variables:
@pytest.fixture(autouse=True)
def _test_env(monkeypatch):
monkeypatch.setenv("AWS_ACCESS_KEY_ID", "testing")
monkeypatch.setenv("AWS_DEFAULT_REGION", "us-east-1")
monkeypatch.setenv("AWS_SECRET_ACCESS_KEY", "testing") # noqa: S105
monkeypatch.setenv("WORKSPACE", "test")
monkeypatch.setenv("LOG_LEVEL", "INFO")
monkeypatch.setenv("DOI_TABLE", "wiley-test")
monkeypatch.setenv("METADATA_URL", "http://example.com/works/")
monkeypatch.setenv("CONTENT_URL", "http://example.com/doi/")
monkeypatch.setenv("BUCKET", "awd")
monkeypatch.setenv("SQS_BASE_URL", "https://queue.amazonaws.com/123456789012/")
monkeypatch.setenv("SQS_INPUT_QUEUE", "mock-input-queue")
monkeypatch.setenv("SQS_OUTPUT_QUEUE", "mock-output-queue")
monkeypatch.setenv("COLLECTION_HANDLE", "123.4/5678")
monkeypatch.setenv("LOG_SOURCE_EMAIL", "noreply@example.com")
monkeypatch.setenv("LOG_RECIPIENT_EMAIL", "mock@mock.mock")
monkeypatch.setenv("RETRY_THRESHOLD", "10")
monkeypatch.setenv("SENTRY_DSN", "None")While the effect is the same in this situation, the fact that it rolls back those changes after the test completes could be helpful when we do want that behavior. For example, if we move into integration tests where actual OS env vars are needed / wanted.
One example is monkeypatch.delenv() which will delete an env var for that test, but rollback after test is complete, which could be helpful. Otherwise, we'd be manipulating the os.environ[] and have to "reset" something for a downstream test.
There was a problem hiding this comment.
I have no concerns about that, your thoughts @jonavellecuerdo?
* Shift list of required env variables to Config class attribute and add attribute definitions * Rename load_config_variables method to load_environment_variables * Remove #type: ignore comments as they are no longer necessary * Replace os.environ calls with monkeypatch.setenv in _test_env fixture
Helpful background context
Adding a
Configclass to this repo to facilitate discussion about we can ultimately add to the Python template repos. Let's refine this!What are the relevant tickets?
Requires Database Migrations?
NO
Includes new or updated dependencies?
YES