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

Retrieve the CR3 download secrets with 1PW #1262

Merged
merged 5 commits into from
Jul 26, 2023

Conversation

mddilley
Copy link
Contributor

@mddilley mddilley commented Jul 25, 2023

Associated issues

cityofaustin/atd-data-tech#12849

Sorry again for all the noise on this one y'all - I think we learned a lot on the way, though. This removes the part of the GitHub Action workflow that pushed updated images of this code for Airflow to run. It also removes the ATD_ETL_CONFIG and config.py file because it was easier to replace those references with os.getenv than it was to resolve when env vars were set in the script and when they were retrieved in that module. The .env only requires your 1PW connect server token, vault id, and 1PW connect server endpoint now.

Testing

URL to test:

Steps to test:

  1. cd to the atd-etl/cr3_download folder in this branch
  2. Follow the directions in the readme there

Ship list

  • Update documentation on how to run the CR3 downloads
  • Remove the CRIS CR3 Download Cookie entry from the API Accessible Secrets vault in 1PW
  • Remove atddocker/vz-cr3-download image from DockerHub
  • Code reviewed
  • Product manager approved

Comment on lines +89 to +92
# ask user for a set of valid cookies for requests to the CRIS website
CRIS_BROWSER_COOKIES = input(
"Please login to CRIS and extract the contents of the Cookie: header and please paste it here:"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🥲

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.

I agree with you that it's a real bummer that this didn't work out on account of the cookie validation system they have in place. I really liked your work on bringing this into Airflow. 😢

But in much brighter news: I used this branch this morning to download the CR3s in production and it worked like a champ. I have a minor suggestion about the docker image, but already as this is, it's an easier daily task thanks to your docker compose stack usage. Big 👍 from me!

Comment on lines 7 to 9
$ docker compose run -it cr3_download python cr3_download.py
$ docker compose run --build -it cr3_download python cr3_download.py
Copy link
Member

Choose a reason for hiding this comment

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

What would you think of adding CMD python /app/cr3_download.py as the last line of the Dockerfile so that the daily invocation becomes docker compose run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! ✨ This is a great idea. I'll add this right now. 🙏

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.

Send it 🥏

@Charlie-Henry Charlie-Henry self-requested a review July 26, 2023 17:45
Copy link
Contributor

@Charlie-Henry Charlie-Henry left a comment

Choose a reason for hiding this comment

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

I was able to build the docker image and paste the 🍪 in. Got a List of 0 crashes needing CR3 download: [] so I don't think I actually tested the 1pass integration because I didn't try to upload anything to S3 or use hasura?

It is in your checklist but I use these docs when I'm on cr-3 duty so those will need to be updated.

🚢 :shipit:

@mddilley
Copy link
Contributor Author

@Charlie-Henry thanks for taking a look at this! Thanks for pointing to those docs too 🙏 And, true, testing without missing pdfs doesn't fully test this. I'm taking Frank's test this morning as confirmation that this will work going forward. ✅

@mddilley
Copy link
Contributor Author

Okie dokie - docs are updated and the rest of the checklist is done. Merging!

@mddilley mddilley merged commit 30fd303 into master Jul 26, 2023
9 checks passed
@mddilley mddilley deleted the md-12849-update-cr3-download branch July 26, 2023 18:17
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