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

Updating tests to use real and fake broker_settings.yaml #216

Closed
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
5 changes: 5 additions & 0 deletions broker/providers/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,18 @@ class Provider(metaclass=ProviderMeta):
_checkout_options = []
_execute_options = []
_fresh_settings = settings.dynaconf_clone()
_settings_id = settings._broker_settings_id
_sensitive_attrs = []

def __init__(self, **kwargs):
self._construct_params = []
cls_name = self.__class__.__name__
logger.debug(f"{cls_name} provider instantiated with {kwargs=}")
self.instance = kwargs.pop(f"{cls_name}", None)
if not self._settings_id == settings._broker_settings_id:
logger.debug("Settings object has been reloaded, reloading provider settings")
Copy link
Member

Choose a reason for hiding this comment

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

What is the case when we need to reload the settings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For running the test suite we need to change the settings file being used. Now that we automatically look for settings in ~/.broker we need to change it to our broker_settings for testing when the tests are running.

self._fresh_settings = settings.dynaconf_clone()
self._settings_id = settings._broker_settings_id
self._validate_settings(self.instance)

def _validate_settings(self, instance_name=None):
Expand Down
35 changes: 22 additions & 13 deletions broker/settings.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import os
import click
import inspect
import uuid
from pathlib import Path
from dynaconf import Dynaconf, Validator
from dynaconf.validator import ValidationError
Expand Down Expand Up @@ -91,19 +92,27 @@ def init_settings(settings_path, interactive=False):
for k in vault_vars:
del os.environ[k]

settings = Dynaconf(
settings_file=str(settings_path.absolute()),
ENVVAR_PREFIX_FOR_DYNACONF="BROKER",
validators=validators,
)
# to make doubly sure, remove the vault loader if set somehow
settings._loaders = [loader for loader in settings._loaders if "vault" not in loader]
os.environ.update(vault_vars)

try:
settings.validators.validate()
except ValidationError as err:
raise ConfigurationError(
f"Configuration error in {settings_path.absolute()}: {err.args[0]}"
settings = Dynaconf(ENVVAR_PREFIX_FOR_DYNACONF="BROKER", validators=validators)
settings._loaders = [loader for loader in settings._loaders if "vault" not in loader]
def load_settings(settings_path):
_settings = Dynaconf(
settings_file=str(settings_path.absolute()),
ENVVAR_PREFIX_FOR_DYNACONF="BROKER",
validators=validators,
)
# to make doubly sure, remove the vault loader if set somehow
_settings._loaders = [loader for loader in _settings._loaders if "vault" not in loader]

os.environ.update(vault_vars)
try:
_settings.validators.validate()
except ValidationError as err:
raise ConfigurationError(
f"Configuration error in {settings_path.absolute()}: {err.args[0]}"
)
settings.clean()
settings.update(_settings.as_dict())
Comment on lines +114 to +115
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to have settings and _settings alongside when one is already living in the module context?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I understand the question. The reason for having the two here is that settings lives in the module context and we use this function to update that settings object with what we get from _settings. I'm pretty sure we were not able to just reset settings to another Dynaconf object because then it would lose context in places that it was already imported in, but I honestly can't remember at this point.

settings._broker_settings_id = str(uuid.uuid4())

load_settings(settings_path)
16 changes: 0 additions & 16 deletions broker_settings.yaml.example
Original file line number Diff line number Diff line change
Expand Up @@ -43,25 +43,9 @@ Container:
Beaker:
hub_url:
max_job_wait: 24h
TestProvider:
instances:
- test1:
foo: "bar"
default: True
- test2:
foo: "baz"
override_envars: True
- bad:
nothing: False
config_value: "something"
# You can set a nickname as a shortcut for arguments
nicks:
rhel7:
workflow: "deploy-base-rhel"
rhel_version: "7.9"
notes: "Requested by broker"
test_nick:
test_action: "fake"
arg1: "abc"
arg2: 123
arg3: True
20 changes: 20 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import os
import pytest
from broker.settings import settings_path, inventory_path, load_settings
import shutil


@pytest.fixture
Expand All @@ -15,3 +17,21 @@ def set_envars(request):
os.environ[request.param[0]] = request.param[1]
yield
del os.environ[request.param[0]]

@pytest.fixture(scope="module")
def temp_settings_and_inventory():
Copy link
Member

Choose a reason for hiding this comment

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

What about using the tmp_path fixture and creating the new files in it? Then setting BROKER_DIRECTORY to tmp_path and unsettling it once the test is done?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could try it if you think that's an improvement over this implementation. @JacobCallahan any thoughts since you helped me with our current solution?

"""Temporarily move the local inventory and settings files, then move them back when done"""
inv_backup_path = None
if inventory_path.exists():
inv_backup_path = inventory_path.rename(f"{inventory_path.absolute()}.bak")
settings_backup_path = settings_path.rename(f"{settings_path.absolute()}.bak")
shutil.copyfile('tests/data/broker_settings.yaml', f'{settings_path.parent}/broker_settings.yaml')
inventory_path.touch()
load_settings(settings_path)
yield
inventory_path.unlink()
settings_path.unlink()
if inv_backup_path:
inv_backup_path.rename(inventory_path)
settings_backup_path.rename(settings_path)
load_settings(settings_path)
54 changes: 54 additions & 0 deletions tests/data/broker_settings.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# Broker settings
# different log levels for file and stdout
logging:
console_level: info
file_level: debug
# Host Settings
# These can be left alone if you're not using Broker as a library
host_username: root
host_password: "<password>"
host_ssh_port: 22
host_ssh_key_filename: "</path/to/the/ssh-key>"
# Provider settings
TestProvider:
instances:
- test1:
foo: "bar"
default: True
- test2:
foo: "baz"
override_envars: True
- bad:
nothing: False
config_value: "something"
AnsibleTower:
base_url: "https://<ansible tower host>/"
# Username is required for both token and password-based authentication
username: "<username>"
# token is the preferred authentication method
token: "<AT personal access token>"
# password: "<plain text password>"
inventory: "inventory name"
release_workflow: "remove-vm"
extend_workflow: "extend-vm"
new_expire_time: "+172800"
workflow_timeout: 3600
results_limit: 50
Container:
host_username: "<username>"
host_password: "<plain text password>"
host_port: None
default: True
runtime: 'docker'
# name used to prefix container names, used to distinguish yourself
# if not set, then your local username will be used
# name_prefix: test
results_limit: 50
auto_map_ports: False
# You can set a nickname as a shortcut for arguments
nicks:
test_nick:
test_action: "fake"
arg1: "abc"
arg2: 123
arg3: True
2 changes: 1 addition & 1 deletion tests/data/cli_scenarios/satlab/checkout_latest_rhel.yaml
Original file line number Diff line number Diff line change
@@ -1 +1 @@
workflow: deploy-base-rhel
workflow: deploy-rhel
2 changes: 1 addition & 1 deletion tests/data/cli_scenarios/satlab/checkout_latest_sat.yaml
Original file line number Diff line number Diff line change
@@ -1 +1 @@
workflow: deploy-sat-jenkins
workflow: deploy-satellite
2 changes: 1 addition & 1 deletion tests/data/cli_scenarios/satlab/checkout_rhel78.yaml
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
workflow: deploy-base-rhel
workflow: deploy-rhel
deploy_rhel_version: "7.8"
2 changes: 2 additions & 0 deletions tests/data/cli_scenarios/satlab/checkout_sat_613.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
workflow: deploy-satellite
deploy_sat_version: "6.13"
2 changes: 0 additions & 2 deletions tests/data/cli_scenarios/satlab/checkout_sat_69.yaml

This file was deleted.

32 changes: 13 additions & 19 deletions tests/functional/test_containers.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from broker import Broker
from broker.commands import cli
from broker.providers.container import Container
from broker.settings import inventory_path
from broker.settings import inventory_path, settings_path

SCENARIO_DIR = Path("tests/data/cli_scenarios/containers")

Expand Down Expand Up @@ -74,23 +74,20 @@ def test_container_e2e():
assert c_host._cont_inst.top()["Processes"]
res = c_host.execute("hostname")
assert res.stdout.strip() == c_host.hostname
loc_settings_path = Path("broker_settings.yaml")
remote_dir = "/tmp/fake"
c_host.session.sftp_write(loc_settings_path.name, f"{remote_dir}/")
c_host.session.sftp_write(settings_path, f"{remote_dir}/")
res = c_host.execute(f"ls {remote_dir}")
assert str(loc_settings_path) in res.stdout
assert str(settings_path.name) in res.stdout
with NamedTemporaryFile() as tmp:
c_host.session.sftp_read(
f"{remote_dir}/{loc_settings_path.name}", tmp.file.name
)
c_host.session.sftp_read(f"{remote_dir}/{settings_path.name}", tmp.file.name)
data = c_host.session.sftp_read(
f"{remote_dir}/{loc_settings_path.name}", return_data=True
f"{remote_dir}/{settings_path.name}", return_data=True
)
assert (
loc_settings_path.read_bytes() == Path(tmp.file.name).read_bytes()
settings_path.read_bytes() == Path(tmp.file.name).read_bytes()
), "Local file is different from the received one"
assert (
loc_settings_path.read_bytes() == data
settings_path.read_bytes() == data
), "Local file is different from the received one (return_data=True)"
assert (
data == Path(tmp.file.name).read_bytes()
Expand All @@ -110,23 +107,20 @@ def test_container_e2e_mp():
assert c_host._cont_inst.top()["Processes"]
res = c_host.execute("hostname")
assert res.stdout.strip() == c_host.hostname
loc_settings_path = Path("broker_settings.yaml")
remote_dir = "/tmp/fake"
c_host.session.sftp_write(loc_settings_path.name, f"{remote_dir}/")
c_host.session.sftp_write(settings_path, f"{remote_dir}/")
res = c_host.execute(f"ls {remote_dir}")
assert str(loc_settings_path) in res.stdout
assert str(settings_path.name) in res.stdout
with NamedTemporaryFile() as tmp:
c_host.session.sftp_read(
f"{remote_dir}/{loc_settings_path.name}", tmp.file.name
)
c_host.session.sftp_read(f"{remote_dir}/{settings_path.name}", tmp.file.name)
data = c_host.session.sftp_read(
f"{remote_dir}/{loc_settings_path.name}", return_data=True
f"{remote_dir}/{settings_path.name}", return_data=True
)
assert (
loc_settings_path.read_bytes() == Path(tmp.file.name).read_bytes()
settings_path.read_bytes() == Path(tmp.file.name).read_bytes()
), "Local file is different from the received one"
assert (
loc_settings_path.read_bytes() == data
settings_path.read_bytes() == data
), "Local file is different from the received one (return_data=True)"
assert (
data == Path(tmp.file.name).read_bytes()
Expand Down
28 changes: 13 additions & 15 deletions tests/functional/test_satlab.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from broker import Broker
from broker.commands import cli
from broker.providers.ansible_tower import AnsibleTower
from broker.settings import inventory_path
from broker.settings import inventory_path, settings_path

SCENARIO_DIR = Path("tests/data/cli_scenarios/satlab")

Expand Down Expand Up @@ -69,21 +69,20 @@ def test_tower_host():
with Broker(workflow="deploy-base-rhel") as r_host:
res = r_host.execute("hostname")
assert res.stdout.strip() == r_host.hostname
loc_settings_path = Path("broker_settings.yaml")
remote_dir = "/tmp/fake"
r_host.session.sftp_write(loc_settings_path.name, f"{remote_dir}/")
r_host.session.sftp_write(settings_path, f"{remote_dir}/")
res = r_host.execute(f"ls {remote_dir}")
assert str(loc_settings_path) in res.stdout
assert str(settings_path.name) in res.stdout
with NamedTemporaryFile() as tmp:
r_host.session.sftp_read(f"{remote_dir}/{loc_settings_path.name}", tmp.file.name)
r_host.session.sftp_read(f"{remote_dir}/{settings_path.name}", tmp.file.name)
data = r_host.session.sftp_read(
f"{remote_dir}/{loc_settings_path.name}", return_data=True
f"{remote_dir}/{settings_path.name}", return_data=True
)
assert (
loc_settings_path.read_bytes() == Path(tmp.file.name).read_bytes()
settings_path.read_bytes() == Path(tmp.file.name).read_bytes()
), "Local file is different from the received one"
assert (
loc_settings_path.read_bytes() == data
settings_path.read_bytes() == data
), "Local file is different from the received one (return_data=True)"
assert data == Path(tmp.file.name).read_bytes(), "Received files do not match"
# test the tail_file context manager
Expand All @@ -100,21 +99,20 @@ def test_tower_host_mp():
for r_host in r_hosts:
res = r_host.execute("hostname")
assert res.stdout.strip() == r_host.hostname
loc_settings_path = Path("broker_settings.yaml")
remote_dir = "/tmp/fake"
r_host.session.sftp_write(loc_settings_path.name, f"{remote_dir}/")
r_host.session.sftp_write(settings_path, f"{remote_dir}/")
res = r_host.execute(f"ls {remote_dir}")
assert str(loc_settings_path) in res.stdout
assert str(settings_path.name) in res.stdout
with NamedTemporaryFile() as tmp:
r_host.session.sftp_read(f"{remote_dir}/{loc_settings_path.name}", tmp.file.name)
r_host.session.sftp_read(f"{remote_dir}/{settings_path.name}", tmp.file.name)
data = r_host.session.sftp_read(
f"{remote_dir}/{loc_settings_path.name}", return_data=True
f"{remote_dir}/{settings_path.name}", return_data=True
)
assert (
loc_settings_path.read_bytes() == Path(tmp.file.name).read_bytes()
settings_path.read_bytes() == Path(tmp.file.name).read_bytes()
), "Local file is different from the received one"
assert (
loc_settings_path.read_bytes() == data
settings_path.read_bytes() == data
), "Local file is different from the received one (return_data=True)"
assert data == Path(tmp.file.name).read_bytes(), "Received files do not match"
# test remote copy from one host to another
Expand Down
6 changes: 6 additions & 0 deletions tests/providers/test_ansible_tower.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ def pop(self, item=None):
return super().pop(item)


@pytest.fixture(scope="module", autouse=True)
def _temp_settings_and_inventory(temp_settings_and_inventory):
"""Temporarily move the local inventory and settings files, then move them back when done"""
pass


@pytest.fixture(scope="function")
def api_stub():
yield AwxkitApiStub()
Expand Down
6 changes: 6 additions & 0 deletions tests/providers/test_container.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ def create_container(self, container_host, **kwargs):
return MockStub(container)


@pytest.fixture(scope="module", autouse=True)
def _temp_settings_and_inventory(temp_settings_and_inventory):
"""Temporarily move the local inventory and settings files, then move them back when done"""
pass


@pytest.fixture(scope="function")
def api_stub():
yield ContainerApiStub()
Expand Down
4 changes: 4 additions & 0 deletions tests/test_broker.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
from broker.providers import test_provider
import pytest

@pytest.fixture(scope="module", autouse=True)
def _temp_settings_and_inventory(temp_settings_and_inventory):
"""Temporarily move the local inventory and settings files, then move them back when done"""
pass

def test_empty_init():
"""Broker should be able to init without any arguments"""
Expand Down
4 changes: 4 additions & 0 deletions tests/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
"my_second_arg": "foo",
}

@pytest.fixture(scope="module", autouse=True)
def _temp_settings_and_inventory(temp_settings_and_inventory):
"""Temporarily move the local inventory and settings files, then move them back when done"""
pass

@pytest.fixture
def tmp_file(tmp_path):
Expand Down
6 changes: 6 additions & 0 deletions tests/test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@
from broker.providers.test_provider import TestProvider


@pytest.fixture(scope="module", autouse=True)
def _temp_settings_and_inventory(temp_settings_and_inventory):
"""Temporarily move the local inventory and settings files, then move them back when done"""
pass


def test_default_settings():
test_provider = TestProvider()
assert test_provider.instance == "test1"
Expand Down
Loading