Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Socrata ETL GitHub Action #1263

Merged
merged 1 commit into from
Jul 27, 2023
Merged

Conversation

mddilley
Copy link
Contributor

@mddilley mddilley commented Jul 27, 2023

Associated issues

Since this ETL queries the production VZ database, we need the production Airflow DAG to run production code only. Otherwise, a schema change in staging (like adding a new column, etc) could break the ETL. This happened when the GraphQL query to get crash data had in_austin_full_purpose in the filters in staging while the production database still uses austin_full_purpose.

See https://austininnovation.slack.com/archives/C013G4RNJ01/p1690432638099059?thread_ts=1690431847.716579&cid=C013G4RNJ01

Testing

URL to test:
n/a

Steps to test:

  1. Give me a sanity check on the logic in this change please 🙏

I read the action as:

  • Runs when code is pushed to master or production branch
  • if there is a code change within atd-etl/socrata_export/**:
    • if the push was from production then update Docker image tagged with production
    • otherwise, update Docker image tagged with development (for Airflow local testing - I realize that I am breaking the convention we started and can change this back to latest if anyone feels strongly)

Ship list

  • Code reviewed
  • Product manager approved

Copy link
Member

@johnclary johnclary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I agree that using latest isn't ideal. Fwiw staging might be a more explicit tag name? In any case take my approval with a grain of salt since i don't do development here :)

Copy link
Member

@frankhereford frankhereford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mike!

@mddilley
Copy link
Contributor Author

@johnclary yep, i also am not sure about the tag. I picked development only because this DAG uses the environment variable to decided which image tag to run (development locally or production). Not sure how useful having the development tag with code from master is but we shall see.

Copy link
Contributor

@roseeichelmann roseeichelmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay thanks for doing this mike, looks good to me!

@johnclary
Copy link
Member

matching the env var seems like a good enough reason to me 🙏

Copy link
Member

@patrickm02L patrickm02L left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go 🚢

@mddilley mddilley merged commit 499f3ac into master Jul 27, 2023
9 checks passed
@mddilley mddilley deleted the md-patch-socrata-etl-gh-action branch July 27, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants