Skip to content

Code review feedback  #1

@erika-tyagi

Description

@erika-tyagi

General

  • I'm sure this is already on your list, but make sure to set up isolated staging and production environments (ideally: different branches that deploy different CFN stacks with different AWS resources feeding into different Shiny apps). This should be easy enough (and basically free, yay serverless!) from the AWS side, and I think because of the shinyapps.io tier you're on, it shouldn't be any additional cost on that front?

README

  • Again, I'm sure this is already on your list, but make sure the README is more fleshed out. Here's a template I set up with Mitchell Thorson that should be helpful for this use case.
  • I'd also consider exporting the Mural image and then adding it to the repo directly (so you don't need a Mural account to view it).

SAM template

  • Update the description here.
  • Consider reducing the scope of the Lambda execution role and GitHub actions role (especially for S3 access - e.g., to restrict their access to only the requisite buckets).
  • Consider setting up S3 lifecycle policies to delete/archive Athena output after a certain period of time (e.g. 24 hours? 7 days?) to save storage costs.

tests/

  • Add instructions for running the tests
  • Add __pycache__ to a .gitignore file.

src/

  • It wasn't totally clear to me whether all of these files are used in the application.
  • For serverless applications of this scope, I think it would be easier to read if there's a single Python script with the handler and then a utils.py script with all other helper functions (rather than multiple Python scripts with a single function).

query/

  • In the requirements.txt file, I'd highly recommend specifying the version of requests and pandas.
  • These are more application-specific questions, but I'm wondering if one hour is the right amount of time for the pre-signed URL expiry. My initial thought is that 24 hours might be more appropriate.
  • Similarly, I wonder if it makes sense to send emails from nccs@urban.org rather than your personal Urban email.
  • There's a handful of relics of EDP code that I think can probably be cleaned up 😊

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions