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

Fix: monitor-only not being set properly when false/no passed in config #285

Merged
Show file tree
Hide file tree
Changes from 3 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
25 changes: 23 additions & 2 deletions landscape/client/deployment.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,20 @@
import subprocess
import sys
import time
from argparse import SUPPRESS
from datetime import datetime
from datetime import timezone
from logging import debug
from argparse import SUPPRESS
from logging import info
from typing import Sequence

from twisted.logger import globalLogBeginner

from landscape import VERSION
from landscape.client import DEFAULT_CONFIG
from landscape.client import GROUP
from landscape.client import USER
from landscape.client import snap_http
from landscape.client import USER
from landscape.client.snap_utils import get_snap_info
from landscape.client.upgraders import UPGRADE_MANAGERS
from landscape.lib import logging
Expand Down Expand Up @@ -304,3 +305,23 @@ def generate_computer_title(auto_enroll_config):
title = hostname

return title


def convert_arg_to_bool(value: str) -> bool:
"""
Converts an argument provided that is in string format
to be a boolean value.
"""
TRUTHY_VALUES = {"true", "yes", "y", "1", "on"}
FALSY_VALUES = {"false", "no", "n", "0", "off"}

if value.lower() in TRUTHY_VALUES:
return True
elif value.lower() in FALSY_VALUES:
return False
else:
info(
"Error. Invalid boolean provided in config or parameters. "
+ "Defaulting to False.",
)
return False
33 changes: 32 additions & 1 deletion landscape/client/tests/test_deployment.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from landscape.client.deployment import BaseConfiguration
from landscape.client.deployment import Configuration
from landscape.client.deployment import convert_arg_to_bool
from landscape.client.deployment import generate_computer_title
from landscape.client.deployment import get_versioned_persist
from landscape.client.deployment import init_logging
Expand Down Expand Up @@ -468,7 +469,7 @@ def test_generate_computer_title_wait_for_serial_no_serial_assertion(
)
self.assertIsNone(title)
mock_debug.assert_called_once_with(
"No serial assertion in snap info {}, waiting..."
"No serial assertion in snap info {}, waiting...",
)

@mock.patch("landscape.client.deployment.debug")
Expand Down Expand Up @@ -601,3 +602,33 @@ def test_generate_computer_title_with_date(
},
)
self.assertEqual(title, "2024-machine")


class ArgConversionTest(LandscapeTest):
"""Tests for `convert_arg_to_bool` function"""

def test_true_values(self):
TRUTHY_VALUES = {"true", "yes", "y", "1", "on", "TRUE", "Yes"}
val = True
for t in TRUTHY_VALUES:
val = convert_arg_to_bool(t)
self.assertTrue(val)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here


def test_false_values(self):
FALSY_VALUES = {"false", "no", "n", "0", "off", "FALSE", "No"}
val = False
for f in FALSY_VALUES:
val = convert_arg_to_bool(f)
self.assertFalse(val)
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want this in the loop - it's OK if the test fails after the first failing instance. We don't have to worry about subtests.

You also won't need to initialize val, then.


@mock.patch("landscape.client.deployment.info")
def test_invalid_values(self, logging):
INVALID_VALUES = {"invalid", "truthy", "2", "exit"}
val = False
for i in INVALID_VALUES:
val = convert_arg_to_bool(i)
logging.assert_called_with(
"Error. Invalid boolean provided in config or parameters. "
+ "Defaulting to False.",
)
self.assertFalse(val)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

21 changes: 20 additions & 1 deletion landscape/client/tests/test_watchdog.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
from twisted.internet.utils import getProcessOutput
from twisted.python.fakepwd import UserDatabase

from landscape.client import USER
import landscape.client.watchdog
from landscape.client import USER
from landscape.client.amp import ComponentConnector
from landscape.client.broker.amp import RemoteBrokerConnector
from landscape.client.reactor import LandscapeReactor
Expand Down Expand Up @@ -1147,13 +1147,32 @@ def test_monitor_only(self):
self.config.load(["--monitor-only"])
self.assertEqual(self.config.get_enabled_daemons(), [Broker, Monitor])

def test_monitor_only_false(self):
self.config.load(["--monitor-only", "false"])
self.assertEqual(
self.config.get_enabled_daemons(),
[Broker, Monitor, Manager],
)

def test_default_daemons(self):
self.config.load([])
self.assertEqual(
self.config.get_enabled_daemons(),
[Broker, Monitor, Manager],
)

@mock.patch("landscape.client.deployment.info")
def test_monitor_only_invalid_entry(self, logging):
self.config.load(["--monitor-only", "invalid-option"])
logging.assert_called_once_with(
"Error. Invalid boolean provided in config or parameters. "
+ "Defaulting to False.",
)
self.assertEqual(
self.config.get_enabled_daemons(),
[Broker, Monitor, Manager],
)


class WatchDogServiceTest(LandscapeTest):
def setUp(self):
Expand Down
9 changes: 6 additions & 3 deletions landscape/client/watchdog.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

The main C{landscape-client} program uses this watchdog.
"""

import errno
import os
import pwd
Expand All @@ -26,13 +25,14 @@
from twisted.internet.error import ProcessExitedAlready
from twisted.internet.protocol import ProcessProtocol

from landscape.client import IS_SNAP
from landscape.client import GROUP
from landscape.client import IS_SNAP
from landscape.client import USER
from landscape.client.broker.amp import RemoteBrokerConnector
from landscape.client.broker.amp import RemoteManagerConnector
from landscape.client.broker.amp import RemoteMonitorConnector
from landscape.client.deployment import Configuration
from landscape.client.deployment import convert_arg_to_bool
from landscape.client.deployment import init_logging
from landscape.client.reactor import LandscapeReactor
from landscape.lib.bootstrap import BootstrapDirectory
Expand Down Expand Up @@ -522,7 +522,10 @@ def make_parser(self):
)
parser.add_argument(
"--monitor-only",
action="store_true",
type=convert_arg_to_bool,
nargs="?",
const=True,
default=False,
help="Don't enable management features. This is "
"useful if you want to run the client as a non-root "
"user.",
Expand Down