Address deploy-agent lint errors (#1642)

* 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

* Remove ruff format

* Also remove djlint-format

.pre-commit-config.yaml

@@ -9,10 +9,8 @@ repos:
     hooks:
       - id: ruff
         args: [ --fix ]
-      - id: ruff-format
   - repo: https://github.com/djlint/djLint
     rev: v1.34.1
     hooks:
-      - id: djlint-reformat-django
       - id: djlint-django
         args: [--ignore="H031"]

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())

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__"):

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,39 +289,39 @@ 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
         if output:
             return output.decode().strip()
         else:
             return None
-    except:
+    except Exception:
         log.error("Error when fetching teletraan configure manager version")
         return None
 

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:

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()))
 

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):
         """

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
 

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()

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 = {

deploy-agent/tests/unit/deploy/utils/test_exec.py

@@ -3,17 +3,16 @@
 # 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.
 # See the License for the specific language governing permissions and
 # 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)

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