Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: dynaconf fixture with environments #8

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ jobs:

- name: Run tests
run: |
pytest
pytest
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra change, unrelated to the changes; I pushed a small commit to fix it, after checking whether other files had trailing spaces


install-test:
name: "test as installed"
Expand Down
58 changes: 58 additions & 0 deletions CONFIGURATION.md
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While added documentation is good, I feel like I should make use of the mkdocs bits currently available (and not used yet), so there is a proper documentation available also in other formats.

I'll work on this, so we can have a better place for the docs.

Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# Configuration and common usecases
## Tests are run in upstream environment

Pytest will run local candlepin service in a container.

Dynaconf will load all configuration from a file `settings.toml`

```bash
$ export ENVIRONMENTS_FOR_DYNACONF=True
$ export ENV_FOR_DYNACONF=upstream
$
$ pytest -v
```
Pytest will run all tests that are available for `upstream`
environment.

There is a section `[upstream]` in the settings file that contains of
all properties that tests require.

## Tests are run in CI environment
Jenkins does not require dynaconf to use environments.
Jenkins provides all config properties directly - using environment
variables.

```bash
$ export ENVIRONMENTS_FOR_DYNACONF=False
$
$ pytest -v
```

## Tests are run in stage environment
Properties that describe a development environment must be stored in a
secret file that is not part of the repo. Default name of the file is
`.secrets.toml`

```bash
$ export ENVIRONMENTS_FOR_DYNACONF=True
$ export ENV_FOR_DYNACONF=stage
$
$ pytest -v
```

`stage` environment is used for most of the tests. It requires all
services to be available.

All properties are defined in a custom file `.secrets.toml` This file
is not part of this repo.
Comment on lines +31 to +47
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this section seems a bit a duplication of the previous one on upstream environment: all it says different is mentioning the different environment name & context; I'd rather make it generic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is difference that upstream environment will be already configured in the repo (ie. all config properties will be in a file settings.toml (or generated automatically from test candlepin). So the most important part of this section is note about .secrets.toml that is not part of the repo and it is necessary to create it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference I see is related to the configuration file for dynaconf; since that will need to be explained/documented regardless of the environment used, then the two different sections become basically redundant.

Please un-duplicate them, thanks.


# Example of `.secrets.toml`

```toml
[stage.candlepin]
hostname=TO_BE_CONFIGURED

[stage.account]
username=TO_BE_REDACTED
password=TO_BE_REDACTED
```
13 changes: 13 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@ and some helper bits:
- `insights_client` -- `insights-client`
- `rhc` -- `rhc`

## Configuration
The library provides dynaconf object to load all config properties
required by tests. see [Dynaconf library](https://dynaconf.com) for
more details.

There are a few cases the library can be used at
1) upstream environment
2) CI environment
3) stage environment
4) production environment

For more details see [Configuration](CONFIGURATION.md)

## License

Distributed under the terms of the `MIT`_ license.
32 changes: 32 additions & 0 deletions pytest_client_tools/config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# SPDX-FileCopyrightText: Red Hat
# SPDX-License-Identifier: MIT

import pytest
from dynaconf import Dynaconf, Validator

#
# it is necessary to use define global variable since dynaconf settings object
# is used even in pytest hooks to get config properties
#
_settings = Dynaconf(
envvar_prefix="PYTEST_CLIENT_TOOLS",
settings_files=["settings.toml", ".secrets.toml"],
)


@pytest.fixture
def settings(scope="session"):
_settings.validators.register(
Validator("candlepin.host"),
Validator("candlepin.port", gt=0, lt=65536, cast=int, is_type_of=int),
Validator("candlepin.prefix", startswith="/"),
Validator("candlepin.insecure", cast=bool, is_type_of=bool),
Validator("candlepin.username"),
Validator(
"candlepin.password",
must_exist=True,
when=Validator("candlepin.username", must_exist=True),
),
)
_settings.validators.validate()
return _settings
Comment on lines +17 to +32
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this greatly reimplements what the existing test_config fixture does already; neither the PR text nor the commit message say why it is duplicated like this (even copy&pasting the existing code), so please explain what is the reason behind this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason for this simplification was that one class in a module is redundant in case test_config. It seems to me that a module file is enough to group functions into one namespace. I vote for keeping it super simple. ie. no hidden logic for a tester, upstream visitor to learn. That is a reason to use only well known tools, libraries rather than creating our own classes with another middleware.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, TestConfig is not redundant, and it does not create any new middleware. As I said, its purpose is to group anything related to the configuration of the test session, i.e. what is read using dynaconf plus additional logic related to that.

20 changes: 18 additions & 2 deletions pytest_client_tools/plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@
import logging
import shutil
import subprocess

jstavel marked this conversation as resolved.
Show resolved Hide resolved
import pytest


jstavel marked this conversation as resolved.
Show resolved Hide resolved
from .candlepin import Candlepin, ping_candlepin
from .insights_client import InsightsClient, INSIGHTS_CLIENT_FILES_TO_SAVE
from .logger import LOGGER
from .podman import Podman


Comment on lines +17 to +18
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra changes, unrelated (please drop)

from .subscription_manager import (
SubscriptionManager,
SUBMAN_FILES_TO_SAVE,
Expand All @@ -22,7 +24,7 @@
from .rhc import Rhc, RHC_FILES_TO_SAVE
from .test_config import TestConfig
from .util import NodeRunningData

from .config import _settings

_MARKERS = {
"candlepin": "tests requiring a self-deployed Candlepin",
Expand Down Expand Up @@ -233,6 +235,11 @@ def pytest_configure(config):
for mark, description in _MARKERS.items():
config.addinivalue_line("markers", f"{mark}: {description}")
config.addinivalue_line("markers", "jira(id): test for jira cards")
config.addinivalue_line(
"markers",
"env(name): required Dynaconf environment for a test",
)

locale.setlocale(locale.LC_ALL, "C.UTF-8")


Expand All @@ -256,3 +263,12 @@ def pytest_runtest_logfinish(nodeid, location):
if node_running_data.logfile.exists():
node_running_data.artifacts.copy(node_running_data.logfile)
logging.getLogger().handlers.remove(node_running_data.handler)


def pytest_runtest_setup(item):
envnames = [mark.args[0] for mark in item.iter_markers(name="env")]
the_environment = _settings.get("env_for_dynaconf") or "development"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is exactly the reason why the TestConfig class (and the test_config fixture) exists: group all the logic related to the "configuration" of tests in a single place; in this case, I'd not want to spread the logic that determines what is the current environment all over the place, but rather implement this once in TestConfig -- something like:

class TestConfig:
    ...
    @property
    def environment(self):
        return _settings.get("env_for_dynaconf") or DEFAULT_ENVIRONMENT

this way:

  • DEFAULT_ENVIRONMENT is used there, or even can be defined inline (without a variable) if there is no use for it outside of this function
  • whatever needs to know what's the current environment can simply query TestConfig.environment without any logic for reading what's the existing one, and determining the fallback

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is 'default_env' property in Dynaconf. It could be used to specify default environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid a class in this case - since Dynaconf does the same. Groupping functions into one namespace can be done by using module file and functions in it. It is easier for me to update doc aka 'this is a dynaconf object. See: dynaconf.org for more details how it works' rather that keep doc about test_class updated. Another reason was that when a test code is shallow (ie no additional middleware - it is easier to debug it and it is easier for an upstream user to understand the code'. It was my main reason for this suggestion - KISS.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said, TestConfig does not replace dnyaconf is any way, but rather uses it underneath (composition). Even the documentation will still mention dynaconf, that is not going to change.

Thanks for the suggestion, but please use TestConfig and test_config.

if envnames and the_environment not in envnames:
pytest.skip(
f"test requires a Dynaconf environment to be one of those: {envnames}",
)