1
\$\begingroup\$

I'm the maintainer of https://github.com/servian/aws-auto-cleanup. I'm by no means a Python expert and hence I'd say I'm way off creating code that would be considered Pythonic. I do however love to tweak and improve code quality where I can.

I'd love for someone to check over a single class of mine and provide some tips where I could improve.

A simple class to look at is:

import sys

import boto3

from src.helper import Helper


class AmplifyCleanup:
    def __init__(self, logging, whitelist, settings, execution_log, region):
        self.logging = logging
        self.whitelist = whitelist
        self.settings = settings
        self.execution_log = execution_log
        self.region = region

        self._client_amplify = None
        self._dry_run = self.settings.get("general", {}).get("dry_run", True)

    @property
    def client_amplify(self):
        if not self._client_amplify:
            self._client_amplify = boto3.client("amplify", region_name=self.region)
        return self._client_amplify

    def run(self):
        self.apps()

    def apps(self):
        """
        Deletes Amplify Apps.
        """

        self.logging.debug("Started cleanup of Amplify Apps.")

        clean = (
            self.settings.get("services", {})
            .get("amplify", {})
            .get("app", {})
            .get("clean", False)
        )
        if clean:
            try:
                resources = self.client_amplify.list_apps().get("apps")
            except:
                self.logging.error("Could not list all Amplify Apps.")
                self.logging.error(sys.exc_info()[1])
                return False

            ttl_days = (
                self.settings.get("services", {})
                .get("amplify", {})
                .get("app", {})
                .get("ttl", 7)
            )

            for resource in resources:
                resource_id = resource.get("name")
                resource_app_id = resource.get("appId")
                resource_date = resource.get("updateTime")
                resource_action = None

                if resource_id not in self.whitelist.get("amplify", {}).get("app", []):
                    delta = Helper.get_day_delta(resource_date)

                    if delta.days > ttl_days:
                        try:
                            if not self._dry_run:
                                self.client_amplify.delete_app(appId=resource_app_id)
                        except:
                            self.logging.error(
                                f"Could not delete Amplify App '{resource_id}'."
                            )
                            self.logging.error(sys.exc_info()[1])
                            resource_action = "ERROR"
                        else:
                            self.logging.info(
                                f"Amplify App '{resource_id}' was last modified {delta.days} days ago "
                                "and has been deleted."
                            )
                            resource_action = "DELETE"
                    else:
                        self.logging.debug(
                            f"Amplify App '{resource_id}' was last modified {delta.days} days ago "
                            "(less than TTL setting) and has not been deleted."
                        )
                        resource_action = "SKIP - TTL"
                else:
                    self.logging.debug(
                        f"Amplify App '{resource_id}' has been whitelisted and has not been deleted."
                    )
                    resource_action = "SKIP - WHITELIST"

                Helper.record_execution_log_action(
                    self.execution_log,
                    self.region,
                    "Amplify",
                    "App",
                    resource_id,
                    resource_action,
                )

            self.logging.debug("Finished cleanup of Amplify Apps.")
            return True
        else:
            self.logging.info("Skipping cleanup of Amplify Apps.")
            return True

All my classes are basically the same general template calling different AWS Boto3 APIs. Hence if I can improve one, I can improve all of them.

My main dislikes are my dictionary retrievals:

clean = (
    self.settings.get("services", {})
    .get("amplify", {})
    .get("app", {})
    .get("clean", False)
)

and

ttl_days = (
    self.settings.get("services", {})
    .get("amplify", {})
    .get("app", {})
    .get("ttl", 7)
)

Should I be moving these to functions that I would call or should I do something else?

\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Welcome to Code Review!

As this is an open source project, some guidelines/maintenance rules might take precedence over the suggestions here. However, a few points I'd like to mention:

  1. Subclassing your cleanups. Looking through the project source, I notice that all AWS/boto3 supported classes have their own duplicates of __init__, with very minimal changes (like referencing a resource instead of low-level client). Using a base class which does a majority of this work for you would probably be more maintainable.

  2. Naming of certain methods appears counter-intuitive. For example, the function apps() should actually read cleanup_apps()?

  3. As you pointed out in the question itself, your primary concern is with the nested dictionary retrievals. Looking at the source, I notice that again, all looks are always from the self.settings dictionary. Perhaps a helper method in parent class (or extracted into the Helper class) could keep it smaller:

    def setting_lookup(self, path, default=None):
        result = self.settings
        for key in path.split("."):
            result = result.get(key)
            if result is None:
                return default
        return result
    

    and the call would be (aws doesn't allow / and . in their resource names):

    clean = self.settings_lookup("services.amplify.app.clean", default=False)
    

    If you think that the jq like lookup is not for you, it can be just sent across as list of consecutive keys.

  4. The current system does not account for paginated results from the list_apps method. From the official docs, a default maximum is 100 resources in an API call, however for cleaning up of more resources, there is no pagination support in the apps() method for the cleanup class.

\$\endgroup\$
1
\$\begingroup\$

To be honest, I don't see much needed change except those dict look-ups you complained about.

Observe that whenever any key in the get chain is missing, the last default will be returned. So you can safely convert that chain to a try/except structure and use the last default value in case of a missing key.

For example:

clean = (
    self.settings.get("services", {})
    .get("amplify", {})
    .get("app", {})
    .get("clean", False)
)

Will be equivalent to:

try:
    clean = self.settings["services"]["amplify"]["app"]["clean"]
except KeyError:
    clean = False
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.