Config class and S3 clients

[ehanson8]

Purpose and background context

This code is largely copied over from wiley-deposits with some minor changes to align it with our current coding practices.

Note: Most of the line changes are from Pipfile.lock

How can a reviewer manually see the effects of these changes?

Not possible yet

Includes new or updated dependencies?

YES

Changes expectations for external applications?

NO

What are the relevant tickets?

• https://mitlibraries.atlassian.net/browse/IN-1108
• https://mitlibraries.atlassian.net/browse/IN-1099

Developer

• All new ENV is documented in README
• All new ENV has been added to staging and production environments
• All related Jira tickets are linked in commit message(s)
• Stakeholder approval has been confirmed (or is not needed)

Code Reviewer(s)

• The commit message is clear and follows our guidelines (not just this PR message)
• There are appropriate tests covering any new functionality
• The provided documentation is sufficient for understanding any new functionality introduced
• Any manual tests have been performed or provided examples verified
• New dependencies are appropriate or there were no changes

[ehanson8]

This is a combination of configure_logger from wiley-deposits and our template config code, there's some repetition and I have a gut feeling there's a better way to do this but didn't land on anything specific, curious about your thoughts

[ghukill]

Here's a possible refactor, removing some redundancies:

def configure_logger(
    self, logger: logging.Logger, stream: StringIO, *, verbose: bool
) -> str:
    logging_format_base = "%(asctime)s %(levelname)s %(name)s.%(funcName)s()"
    logger.addHandler(logging.StreamHandler(stream))

    if verbose:
        log_method, log_level = logger.debug, logging.DEBUG
        template = logging_format_base + " line %(lineno)d: %(message)s"
        for handler in logging.root.handlers:
            handler.addFilter(logging.Filter("dsc"))
    else:
        log_method, log_level = logger.info, logging.INFO
        template = logging_format_base + ": %(message)s"

    logger.setLevel(log_level)
    logging.basicConfig(format=template)
    logger.addHandler(logging.StreamHandler(stream))
    log_method(f"{logging.getLevelName(logger.getEffectiveLevel())}")

    return (
        f"Logger '{logger.name}' configured with level="
        f"{logging.getLevelName(logger.getEffectiveLevel())}"
    )

At a glance, what makes this hard to refactor are the tests:

• test_configure_logger_not_verbose
• test_configure_logger_verbose

which attempt to read from the stream handler that is setup. That's what the log_method, log_level = do above, is get the appropriate logging method to use for log_method(f"{logging.getLevelName(logger.getEffectiveLevel())}") which satisfies these tests.

My feeling: good enough for now, but globally I think we could improve our logging setups. The presence of logging.basicConfig() is kind of a red flag unto itself. But, I think this is mostly consistent with other projects, so value in that.

For example: I'm not sure that returning a string is ideal. Perhaps this method could just log what it has done, and return None. But this would require updating the CLI, tests, etc. that are expecting a string response. And as mentioned, this is consistent with other apps.

[jonavellecuerdo]

Still need review this at a deeper level but figured I'd pass along a discussion @ghukill and I had on a previous PR I worked on re: excluding configure_sentry and configure_logger from the Config class:

From @ghukill :

• I like having a Config class that I can put complex behavior on for static-esque values I want to access through the application

• I'm indifferent to configure_logger and configure_sentry on there / they feel somewhat disconnected from the essence of the application, which I attribute to the Config class. Especially sentry. Logging is a little more difficult for me... but then again logging is always kind of difficult. \ Of course, if those methods (configure_sentry and configure_logger) are on it, they don't need to be called.... but if I'm not supposed to call configure_sentry from anywhere in the app, then maybe it doesn't belong on that global Config object?
  This is 5:00pm talking, no much reasoning, but just my knee-jerk thinking.

[ehanson8]

Great refactor, thanks! I was hoping this would prompt a larger conversation about the template/config class because I think it needs work as well, maybe a future DataEng meeting topic?

[ghukill]

I think a future DataEng topic could be good, for sure. But I think it could/should extend longer than 30 minute meeting w/ CB.

I have two somewhat conflicting feelings at the moment re: config and logging:

1. it's okay for projects to tweak our established approach as needed
2. it's okay if slightly awkward or sub-optimal insofar as it's consistent with other repos

I think it'd be easy for any one project to grind to a halt trying to get it perfect, when we'll always have tension with, "but others don't do it that way..."

Until we do take a long and thoughtful look at our config + logging approach for python projects, I'm pretty okay with allowing projects to try and strike a balance between feelings #1 and #2 above.

[ghukill]

Couple of comments and a suggestion for a docstring update, but none blocking! Looking like good foundation until the more interesting bits come.

[ghukill]

Here's a possible refactor, removing some redundancies:

def configure_logger(
    self, logger: logging.Logger, stream: StringIO, *, verbose: bool
) -> str:
    logging_format_base = "%(asctime)s %(levelname)s %(name)s.%(funcName)s()"
    logger.addHandler(logging.StreamHandler(stream))

    if verbose:
        log_method, log_level = logger.debug, logging.DEBUG
        template = logging_format_base + " line %(lineno)d: %(message)s"
        for handler in logging.root.handlers:
            handler.addFilter(logging.Filter("dsc"))
    else:
        log_method, log_level = logger.info, logging.INFO
        template = logging_format_base + ": %(message)s"

    logger.setLevel(log_level)
    logging.basicConfig(format=template)
    logger.addHandler(logging.StreamHandler(stream))
    log_method(f"{logging.getLevelName(logger.getEffectiveLevel())}")

    return (
        f"Logger '{logger.name}' configured with level="
        f"{logging.getLevelName(logger.getEffectiveLevel())}"
    )

At a glance, what makes this hard to refactor are the tests:

• test_configure_logger_not_verbose
• test_configure_logger_verbose

which attempt to read from the stream handler that is setup. That's what the log_method, log_level = do above, is get the appropriate logging method to use for log_method(f"{logging.getLevelName(logger.getEffectiveLevel())}") which satisfies these tests.

My feeling: good enough for now, but globally I think we could improve our logging setups. The presence of logging.basicConfig() is kind of a red flag unto itself. But, I think this is mostly consistent with other projects, so value in that.

For example: I'm not sure that returning a string is ideal. Perhaps this method could just log what it has done, and return None. But this would require updating the CLI, tests, etc. that are expecting a string response. And as mentioned, this is consistent with other apps.

[jonavellecuerdo]

A couple small change requests, but it's looking good! Will inspect at a deeper level via second pass.

[jonavellecuerdo]

Still need review this at a deeper level but figured I'd pass along a discussion @ghukill and I had on a previous PR I worked on re: excluding configure_sentry and configure_logger from the Config class:

From @ghukill :

• I like having a Config class that I can put complex behavior on for static-esque values I want to access through the application

• I'm indifferent to configure_logger and configure_sentry on there / they feel somewhat disconnected from the essence of the application, which I attribute to the Config class. Especially sentry. Logging is a little more difficult for me... but then again logging is always kind of difficult. \ Of course, if those methods (configure_sentry and configure_logger) are on it, they don't need to be called.... but if I'm not supposed to call configure_sentry from anywhere in the app, then maybe it doesn't belong on that global Config object?
  This is 5:00pm talking, no much reasoning, but just my knee-jerk thinking.

[jonavellecuerdo]

Curious whether archiving the metadata file will be required for workflows other than Wiley deposits? 🤔 If an archive/ folder will exist for every workflow bucket, I think the function name is fine as-is, otherwise, it might be good to rename the function so it is more universal?

[ehanson8]

I believe so, we probably want an archived/processed path in all of the workflows to prevent re-processing of files

[coveralls]

Pull Request Test Coverage Report for Build 12202770589

Details

• 64 of 68 (94.12%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-4.8%) to 95.238%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
dsc/config.py	35	37	94.59%
dsc/s3.py	22	24	91.67%

Totals
Change from base Build 12183247713:	-4.8%
Covered Lines:	80
Relevant Lines:	84

---

💛 - Coveralls