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

Conversation

david-mclain
Copy link
Contributor

Manual Testing

Verify Bug

  • Run new testcases provided (current branch) checking for when false parameter is sent in using old watchdog file
  • Test cases will fail as Manager daemon is not spawned

Verify Fix

  • Run same testcases as above using new watchdog
  • See that Manager daemon is spawned in cases where false is sent as parameter

@david-mclain david-mclain requested a review from srunde3 November 14, 2024 20:30
Copy link
Contributor

@srunde3 srunde3 left a comment

Choose a reason for hiding this comment

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

Left note inline for a refactor + some unit tests.

landscape/client/watchdog.py Outdated Show resolved Hide resolved
@david-mclain david-mclain requested a review from srunde3 November 27, 2024 17:35
Copy link
Contributor

@srunde3 srunde3 left a comment

Choose a reason for hiding this comment

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

small notes inline for the tests.

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.

"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

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

Copy link
Contributor

@srunde3 srunde3 left a comment

Choose a reason for hiding this comment

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

LGTM 🚢 🦛

Manual testing went well - I ran scripts/landscape-client with various --monitor-only values to confirm that Manager plugins only run when it's false.

@david-mclain david-mclain merged commit 3e4d447 into canonical:main Nov 27, 2024
5 checks passed
@david-mclain david-mclain deleted the lndeng-1804-fix-config-parsing-monitor-only branch December 2, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants