Closed jameshadfield closed 6 months ago
Talked with James in our 1:1. This is ready for me to review. He chose not to implement a new ResourceVersion class (as in Source → Resource → ResourceVersion → SubResource) as we'd talked about two weeks ago, but instead make the existing Resource class version-aware.
I rebased this onto current master
and made the appropriate changes to the server's IAM policies managed by Terraform: 4a0367f. I also dropped 5a33a9e, which was marked for dropping and which we don't want to keep.
@jameshadfield Thinking about my review and suggestions some more, I wanted to offer to implement some of the larger changes I've suggested and then walk you thru them if it'd be more clear for me to do that rather than just discuss them. Not a problem if not. Totally your preference here.
@tsibley could you re-review this at your convenience? All comments should have been addressed, except for (i) log statements which I'll remove later and (ii) the definition of "closest" / redirect to closest code which I'll defer until the wider group makes a decision on desired behavior here. The S3 resources index has been updated to the new format.
I wanted to offer to implement some of the larger changes I've suggested and then walk you thru them if it'd be more clear for me to do that rather than just discuss them
Thanks, but I'd prefer to discuss the concepts rather than read through an implementation. For what it's worth, none of the changes were large in terms of code changes, but conceptually I guess there are a few changes.
Some other todos:
master
since this was branched. Probably swap away from v2 SDK and add necessary functions to src/s3.js
?Some of these are deferrable but they all need doing sooner than later.
@tsibley could you work through your comments when you have a chance? I believe I've addressed them all. (And please resolve the ones you feel are addressed, else with this many comments the PR becomes hard to read through.)
Adopt AWS config support added to master since this was branched. Probably swap away from v2 SDK and add necessary functions to src/s3.js?
Could you add an in-line comment regarding this please. I presume you're talking about the AWS calls within updateResourceVersions
?
@jameshadfield Yep. Was updating myself a bit on this today. Will continue to dive in more.
Could you add an in-line comment regarding this please. I presume you're talking about the AWS calls within
updateResourceVersions
?
Yes, those. Comment added.
I've done another full review here, including the new code and revisiting past comments and resolving them as appropriate.
As well as addressing all the open conversations above, I've added a large number of tests to give confidence to the behaviour across sources and resource types. I'm going to merge this now, but feel free to make suggestions which we can address in future PRs. There's more work I'd like to do now that the server has access to the resource index. Many thanks to the reviewers - it's been a big PR, almost certainly with the most comments of any in Nextstrain's history.
P.S. I couldn't convince Heroku to rebuild the review app (unrelated to the contents of this PR), but I did a final round of testing via dev.nextstrain.org, which has the benefit of allowing logins.
See commit messages for details. Some URLs as an example:
Ready for review, but there are a few things before this can be merged: