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 base image version. #76

Merged

Conversation

rasmi
Copy link
Contributor

@rasmi rasmi commented Sep 12, 2023

This updates the Docker base image to the latest current version. This fixes build issues (removing the need for #74, #75), but I am unsure if the scripts output the same results given version changes in libraries.

@rasmi rasmi requested a review from a team as a code owner September 12, 2023 21:15
@TylerMatteo
Copy link
Contributor

Hello @rasmi! Thank you for opening some pull requests on this repo. As you've encountered, we've been having issues running these scripts recently. I'd love to merge this change into a branch here and test the output. I created an update-image-version branch off of main in this repo. Could you repoint this PR at that branch? Merging to that branch will allow us to do a "dry run" of your changes before landing them in main. Thanks!

@rasmi
Copy link
Contributor Author

rasmi commented Sep 13, 2023

Will do! How do you all validate the output of the script? If there's a way to compare the outputs of the last known good version with the updated base image running on the same data, would that be acceptable? Or are there more data-correctness-focused tests? I saw this repo but I'm not sure if it's still the way this script is validated. Thanks for the guidance!

@rasmi rasmi changed the base branch from main to update-image-version September 13, 2023 18:49
@TylerMatteo
Copy link
Contributor

Will do! How do you all validate the output of the script? If there's a way to compare the outputs of the last known good version with the updated base image running on the same data, would that be acceptable? Or are there more data-correctness-focused tests? I saw this repo but I'm not sure if it's still the way this script is validated. Thanks for the guidance!

We have some basic sanity tests built into the scripts but in addition to that, we'll test this by comparing to older outputs and attempting to load the data into a Geosearch deployment. I'm going to merge which will kick off CI to at least tell us if the image builds and the scripts execute successfully. I should be getting back to you some time next week after doing some testing.

@TylerMatteo TylerMatteo merged commit 77f6271 into NYCPlanning:update-image-version Sep 14, 2023
@rasmi rasmi deleted the update-image-version branch September 14, 2023 13:45
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.

2 participants