From a3867e8f2baec0d13693a1a37494a03ef6eb7f1e Mon Sep 17 00:00:00 2001 From: jamesstottmoj Date: Thu, 14 Nov 2024 15:04:06 +0000 Subject: [PATCH 1/4] Added potential fix to ignore particular helm errors that are causing noises in Sentry --- controlpanel/api/helm.py | 28 +++++++++++++++++++++++++++- tests/api/test_helm.py | 21 +++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/controlpanel/api/helm.py b/controlpanel/api/helm.py index c3978da80..0d9d01907 100644 --- a/controlpanel/api/helm.py +++ b/controlpanel/api/helm.py @@ -14,6 +14,10 @@ # Cache helm repository metadata for 5 minutes (expressed as seconds). CACHE_FOR_MINUTES = 5 * 60 +ERRORS_TO_IGNORE = [ + "release: already exists", + "uninstallation completed with 1 error(s): uninstall: failed to purge the release", +] def get_repo_path(): @@ -87,7 +91,7 @@ def _execute(*args, **kwargs): stderr = proc.stderr.read() log.warning(stderr) log.warning(stdout) - if "error" in stderr.lower() or "error" in stdout.lower(): + if should_raise_error(stderr, stdout): raise HelmError(stderr) except subprocess.CalledProcessError as proc_ex: # Subprocess specific exception handling should capture stderr too. @@ -117,6 +121,28 @@ def _execute(*args, **kwargs): return proc +def should_raise_error(stderr, stdout): + + if should_ignore_error(stderr): + return False + + if should_ignore_error(stdout): + return False + + return True + + +def should_ignore_error(error_string): + lower_error_string = error_string.lower() + + if "error" in lower_error_string: + for error in ERRORS_TO_IGNORE: + if error in lower_error_string: + return True + + return False + + def update_helm_repository(force=False): """ Updates the helm repository and returns a dictionary representation of diff --git a/tests/api/test_helm.py b/tests/api/test_helm.py index ea6a61668..9ab31eefa 100644 --- a/tests/api/test_helm.py +++ b/tests/api/test_helm.py @@ -260,3 +260,24 @@ def test_list_releases_with_namespace(): "qux", ] mock_execute.assert_called_once_with("list", "-aq", "--namespace", "some-ns") + + +@pytest.mark.parametrize( + "stderr, stdout, expected", + [ + ("Error: release: already exists", "All good", False), + ("Error: Something that should throw", "All good", True), + ("", "Error: Something that should throw", True), + ( + ( + "Error: uninstallation completed with 1 error(s): " + "uninstall: Failed to purge the release" + ), + "All good", + False, + ), + ], +) +def test_should_raise_error(stderr, stdout, expected): + result = helm.should_raise_error(stderr, stdout) + assert result == expected From 7c72dead7b7e8a74f76c8b1ef4cd15ea1df3a7ac Mon Sep 17 00:00:00 2001 From: jamesstottmoj Date: Thu, 14 Nov 2024 16:00:04 +0000 Subject: [PATCH 2/4] Ran black --- tests/api/test_helm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/api/test_helm.py b/tests/api/test_helm.py index 9ab31eefa..bd2d90fb9 100644 --- a/tests/api/test_helm.py +++ b/tests/api/test_helm.py @@ -270,7 +270,7 @@ def test_list_releases_with_namespace(): ("", "Error: Something that should throw", True), ( ( - "Error: uninstallation completed with 1 error(s): " + "Error: uninstallation completed with 1 error(s): " "uninstall: Failed to purge the release" ), "All good", From a22083ca4cb4ed6f022ce7792a7999ef94c36d66 Mon Sep 17 00:00:00 2001 From: jamesstottmoj Date: Thu, 14 Nov 2024 16:40:56 +0000 Subject: [PATCH 3/4] Refactored check --- controlpanel/api/helm.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/controlpanel/api/helm.py b/controlpanel/api/helm.py index 0d9d01907..0c43886ca 100644 --- a/controlpanel/api/helm.py +++ b/controlpanel/api/helm.py @@ -123,10 +123,7 @@ def _execute(*args, **kwargs): def should_raise_error(stderr, stdout): - if should_ignore_error(stderr): - return False - - if should_ignore_error(stdout): + if should_ignore_error(stderr) or should_ignore_error(stdout): return False return True From f8d53eb955111dbc547e12f5a008491af45b9121 Mon Sep 17 00:00:00 2001 From: jamesstottmoj Date: Thu, 14 Nov 2024 16:51:36 +0000 Subject: [PATCH 4/4] Fixed bad logic --- controlpanel/api/helm.py | 14 ++++++++------ tests/api/test_helm.py | 9 +++++---- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/controlpanel/api/helm.py b/controlpanel/api/helm.py index 0c43886ca..e6b704961 100644 --- a/controlpanel/api/helm.py +++ b/controlpanel/api/helm.py @@ -122,20 +122,22 @@ def _execute(*args, **kwargs): def should_raise_error(stderr, stdout): + lower_error_string = stderr.lower() + lower_out_string = stdout.lower() + if "error" not in lower_error_string and "error" not in lower_out_string: + return False - if should_ignore_error(stderr) or should_ignore_error(stdout): + if should_ignore_error(lower_error_string) or should_ignore_error(lower_out_string): return False return True def should_ignore_error(error_string): - lower_error_string = error_string.lower() - if "error" in lower_error_string: - for error in ERRORS_TO_IGNORE: - if error in lower_error_string: - return True + for error in ERRORS_TO_IGNORE: + if error in error_string: + return True return False diff --git a/tests/api/test_helm.py b/tests/api/test_helm.py index bd2d90fb9..a8b834f3b 100644 --- a/tests/api/test_helm.py +++ b/tests/api/test_helm.py @@ -263,11 +263,12 @@ def test_list_releases_with_namespace(): @pytest.mark.parametrize( - "stderr, stdout, expected", + "stderr, stdout, raise_error", [ ("Error: release: already exists", "All good", False), ("Error: Something that should throw", "All good", True), - ("", "Error: Something that should throw", True), + ("All good", "Error: Something that should throw", True), + ("All good", "All good", False), ( ( "Error: uninstallation completed with 1 error(s): " @@ -278,6 +279,6 @@ def test_list_releases_with_namespace(): ), ], ) -def test_should_raise_error(stderr, stdout, expected): +def test_should_raise_error(stderr, stdout, raise_error): result = helm.should_raise_error(stderr, stdout) - assert result == expected + assert result == raise_error