From 83f6c0040876ee5355a8ba89e48518b91823d09e Mon Sep 17 00:00:00 2001 From: Omar Soriano Date: Mon, 3 Jun 2024 17:08:33 +0000 Subject: [PATCH] Address deploy-agent lint errors `ruff` was recently introduced to the precommit checks. This PR addresses the existing checks for deploy-agent: * Add `ruff` command to `tox.ini` * Run `ruff check --fix` * Run `ruff check --fix --unsafe-fixes` * Verified the fixes were safe * Use explicit exceptions * Remove unused variables and methods * Remove trailing ws --- deploy-agent/deployd/agent.py | 6 ++--- deploy-agent/deployd/common/types.py | 10 +++---- deploy-agent/deployd/common/utils.py | 26 +++++++++---------- deploy-agent/deployd/download/downloader.py | 4 +-- deploy-agent/deployd/staging/transformer.py | 6 ++--- .../unit/deploy/client/test_base_client.py | 6 ++--- .../tests/unit/deploy/client/test_client.py | 3 +-- .../download/test_s3_download_helper.py | 8 ------ .../unit/deploy/staging/test_transformer.py | 5 ++-- .../tests/unit/deploy/utils/test_exec.py | 7 +++-- deploy-agent/tox.ini | 5 ++++ 11 files changed, 40 insertions(+), 46 deletions(-) diff --git a/deploy-agent/deployd/agent.py b/deploy-agent/deployd/agent.py index b41b83ace4..ad5a28b5a0 100644 --- a/deploy-agent/deployd/agent.py +++ b/deploy-agent/deployd/agent.py @@ -132,7 +132,7 @@ def serve_build(self) -> None: # for each service, we check the container health status log.info(f"the current service is: {status.report.envName}") try: - if status.report.redeploy == None: + if status.report.redeploy is None: status.report.redeploy = 0 healthStatus = get_container_health_info(status.build_info.build_commit, status.report.envName, status.report.redeploy) if healthStatus: @@ -145,7 +145,7 @@ def serve_build(self) -> None: status.report.containerHealthStatus = healthStatus status.report.state = None if "unhealthy" not in healthStatus: - if status.report.wait == None or status.report.wait > 5: + if status.report.wait is None or status.report.wait > 5: status.report.redeploy = 0 status.report.wait = 0 else: @@ -228,7 +228,7 @@ def serve_forever(self) -> None: while True: try: self.serve_build() - except: + except Exception: log.exception("Deploy Agent got exception: {}".format(traceback.format_exc())) finally: time.sleep(self._config.get_daemon_sleep_time()) diff --git a/deploy-agent/deployd/common/types.py b/deploy-agent/deployd/common/types.py index 3dd9e9037f..a2eb44d5ec 100644 --- a/deploy-agent/deployd/common/types.py +++ b/deploy-agent/deployd/common/types.py @@ -3,9 +3,9 @@ # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at -# +# # http://www.apache.org/licenses/LICENSE-2.0 -# +# # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -52,11 +52,11 @@ class DeployType(object): RESTART = 'RESTART' STOP = 'STOP' -class DeployErrorSource(object): +class DeployErrorSource(object): TELEFIG = 'TELEFIG' UNKNOWN = 'UNKNOWN' -class DeployError(object): +class DeployError(object): TELEFIG_UNAVAILABLE = 'TELEFIG_UNAVAILABLE' UNKNOWN = 'UNKNOWN' @@ -197,7 +197,7 @@ def load_from_json(self, json_value) -> None: def to_json(self) -> dict: json = {} for key, value in self.__dict__.items(): - if type(value) is dict: + if isinstance(value, dict): json[key] = {k: v for k, v in value.items()} elif value: if hasattr(value, "__dict__"): diff --git a/deploy-agent/deployd/common/utils.py b/deploy-agent/deployd/common/utils.py index e573d7ea92..b6b63e9294 100644 --- a/deploy-agent/deployd/common/utils.py +++ b/deploy-agent/deployd/common/utils.py @@ -218,7 +218,7 @@ def get_info_from_facter(keys) -> Optional[dict]: else: log.warn("Got empty output from facter by keys {}".format(keys)) return None - except: + except Exception: log.exception("Failed to get info from facter by keys {}".format(keys)) return None @@ -231,17 +231,17 @@ def redeploy_check_for_container(labels, service, redeploy) -> int: if redeploy < max_retry: return redeploy + 1 return 0 - + def redeploy_check_for_non_container(commit, service, redeploy, healthcheckConfigs): if healthcheckConfigs.get("HEALTHCHECK_REDEPLOY_WHEN_UNHEALTHY") == "True": log.info(f"Auto redeployment is enabled on service {service}") max_retry = int(healthcheckConfigs.get("HEALTHCHECK_REDEPLOY_MAX_RETRY", REDEPLOY_MAX_RETRY)) if redeploy < max_retry: log.info(f"redeploy is {redeploy}; max retry is {max_retry}") - create_sc_increment(name='deployd.service_health_status', + create_sc_increment(name='deployd.service_health_status', tags={"status": "redeploy", "service": service, "commit": commit}) return "redeploy-" + str(redeploy + 1) - create_sc_increment(name='deployd.service_health_status', + create_sc_increment(name='deployd.service_health_status', tags={"status": "unhealthy", "service": service, "commit": commit}) return service + ":unhealthy" @@ -261,13 +261,13 @@ def redeploy_check_without_container_status(commit, service, redeploy): try: resp = requests.get(url) if resp.status_code >= 200 and resp.status_code < 300: - create_sc_increment(name='deployd.service_health_status', + create_sc_increment(name='deployd.service_health_status', tags={"status": "healthy", "service": service, "commit": commit}) return service + ":healthy" except requests.ConnectionError: return redeploy_check_for_non_container(commit, service, redeploy, healthcheckConfigs) return redeploy_check_for_non_container(commit, service, redeploy, healthcheckConfigs) - + def get_container_health_info(commit, service, redeploy) -> Optional[str]: try: log.info(f"Get health info for service {service} with commit {commit}") @@ -289,31 +289,31 @@ def get_container_health_info(commit, service, redeploy) -> Optional[str]: labels = parts[2].split(',') ret = redeploy_check_for_container(labels, service, redeploy) if ret > 0: - create_sc_increment(name='deployd.service_health_status', + create_sc_increment(name='deployd.service_health_status', tags={"status": "redeploy", "service": service, "commit": commit}) return "redeploy-" + str(ret) result.append(f"{name}:{status}") - except: + except Exception: continue returnValue = ";".join(result) if result else None if returnValue and "unhealthy" in returnValue: - create_sc_increment(name='deployd.service_health_status', + create_sc_increment(name='deployd.service_health_status', tags={"status": "unhealthy", "service": service, "commit": commit}) elif returnValue and "unhealthy" not in returnValue: - create_sc_increment(name='deployd.service_health_status', + create_sc_increment(name='deployd.service_health_status', tags={"status": "healthy", "service": service, "commit": commit}) if returnValue: return returnValue # if no check happens for the current service, check if it is a non container with healthcheck enabled return redeploy_check_without_container_status(commit, service, redeploy) - except: + except Exception: log.error(f"Failed to get container health info with commit {commit}") return None def get_telefig_version() -> Optional[str]: if not IS_PINTEREST: - return None + return None try: cmd = ['configure-serviceset', '-v'] output = subprocess.run(cmd, check=True, stdout=subprocess.PIPE).stdout @@ -321,7 +321,7 @@ def get_telefig_version() -> Optional[str]: return output.decode().strip() else: return None - except: + except Exception: log.error("Error when fetching teletraan configure manager version") return None diff --git a/deploy-agent/deployd/download/downloader.py b/deploy-agent/deployd/download/downloader.py index baf0dfa7da..5029dc7d62 100644 --- a/deploy-agent/deployd/download/downloader.py +++ b/deploy-agent/deployd/download/downloader.py @@ -119,10 +119,10 @@ def download(self) -> int: with open(extracted_file, 'w'): pass log.info("Successfully extracted {} to {}".format(local_full_fn, working_dir)) - except tarfile.TarError as e: + except tarfile.TarError: status = Status.FAILED log.exception("Failed to extract tar files") - except OSError as e: + except OSError: status = Status.FAILED log.exception("Failed to extract files. OSError:") except Exception: diff --git a/deploy-agent/deployd/staging/transformer.py b/deploy-agent/deployd/staging/transformer.py index d020b8c693..7d0ab04add 100644 --- a/deploy-agent/deployd/staging/transformer.py +++ b/deploy-agent/deployd/staging/transformer.py @@ -3,9 +3,9 @@ # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at -# +# # http://www.apache.org/licenses/LICENSE-2.0 -# +# # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -70,7 +70,7 @@ def _translate(self, from_path, to_path) -> None: res = s.safe_substitute(self._dictionary) with open(to_path, 'w') as f: f.write(res) - except: + except Exception: log.error('Fail to translate script {}, stacktrace: {}'.format(from_path, traceback.format_exc())) diff --git a/deploy-agent/tests/unit/deploy/client/test_base_client.py b/deploy-agent/tests/unit/deploy/client/test_base_client.py index 88463d278c..c4aada1cdc 100644 --- a/deploy-agent/tests/unit/deploy/client/test_base_client.py +++ b/deploy-agent/tests/unit/deploy/client/test_base_client.py @@ -3,9 +3,9 @@ # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at -# +# # http://www.apache.org/licenses/LICENSE-2.0 -# +# # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -28,7 +28,7 @@ class MyClient(BaseClient): pass with self.assertRaises(TypeError): - myclient = MyClient() + MyClient() def test_abc_equivalent_to_old(self): """ diff --git a/deploy-agent/tests/unit/deploy/client/test_client.py b/deploy-agent/tests/unit/deploy/client/test_client.py index eaa3313f54..7dd5052723 100644 --- a/deploy-agent/tests/unit/deploy/client/test_client.py +++ b/deploy-agent/tests/unit/deploy/client/test_client.py @@ -1,8 +1,7 @@ -import mock import unittest from tests import TestCase -from deployd.client.client import Client +from deployd.client.client import Client from deployd.client.base_client import BaseClient from deployd.common.config import Config diff --git a/deploy-agent/tests/unit/deploy/download/test_s3_download_helper.py b/deploy-agent/tests/unit/deploy/download/test_s3_download_helper.py index ce06819f72..7e81ac43e1 100644 --- a/deploy-agent/tests/unit/deploy/download/test_s3_download_helper.py +++ b/deploy-agent/tests/unit/deploy/download/test_s3_download_helper.py @@ -49,13 +49,5 @@ def test_validate_url_without_allow_list(self, mock_get_s3_download_allow_list): self.assertTrue(result) mock_get_s3_download_allow_list.assert_called_once() - @mock.patch('deployd.common.config.Config.get_s3_download_allow_list') - def test_validate_url_without_allow_list(self, mock_get_s3_download_allow_list): - mock_get_s3_download_allow_list.return_value = [] - result = self.downloader.validate_source() - - self.assertTrue(result) - mock_get_s3_download_allow_list.assert_called_once() - if __name__ == '__main__': unittest.main() diff --git a/deploy-agent/tests/unit/deploy/staging/test_transformer.py b/deploy-agent/tests/unit/deploy/staging/test_transformer.py index 20d1837fb4..8c2bf7f4a5 100644 --- a/deploy-agent/tests/unit/deploy/staging/test_transformer.py +++ b/deploy-agent/tests/unit/deploy/staging/test_transformer.py @@ -3,9 +3,9 @@ # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at -# +# # http://www.apache.org/licenses/LICENSE-2.0 -# +# # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -100,7 +100,6 @@ def test_translate2(self): transformer.transform_scripts(self.template_dir, self.template_dir, self.script_dir) fn1 = os.path.join(self.script_dir, 'test1') - fn2 = os.path.join(self.script_dir, 'test2') value1 = "foo-foo-foo" values = { diff --git a/deploy-agent/tests/unit/deploy/utils/test_exec.py b/deploy-agent/tests/unit/deploy/utils/test_exec.py index bd80ee3a29..4e9939790f 100644 --- a/deploy-agent/tests/unit/deploy/utils/test_exec.py +++ b/deploy-agent/tests/unit/deploy/utils/test_exec.py @@ -3,9 +3,9 @@ # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at -# +# # http://www.apache.org/licenses/LICENSE-2.0 -# +# # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -13,7 +13,6 @@ # limitations under the License. import os -import time import signal import tempfile import unittest @@ -142,7 +141,7 @@ def test_run_command_with_shutdown_timeout(self): executor.MAX_SLEEP_INTERVAL = 5 executor.MAX_TAIL_BYTES = 10240 executor.TERMINATE_TIMEOUT = 0 - deploy_report = executor.run_cmd(cmd=cmd) + executor.run_cmd(cmd=cmd) self.assertEqual(os.killpg.call_count, 2) calls = [mock.call(mock.ANY, signal.SIGTERM), mock.call(mock.ANY, signal.SIGKILL)] os.killpg.assert_has_calls(calls) diff --git a/deploy-agent/tox.ini b/deploy-agent/tox.ini index 4b3576faa8..10d154428a 100644 --- a/deploy-agent/tox.ini +++ b/deploy-agent/tox.ini @@ -6,6 +6,11 @@ recreate=True deps=-rrequirements_test.txt commands=py.test --cov=deployd {posargs} +[testenv:ruff] +description = run ruff +deps = ruff +commands = ruff {posargs} + [gh-actions] python = 3.6: py36