Add checksum harvesting for Amazon ECR images in workflows #4376
Conversation
|
Will also need to investigate caching for |
Codecov Report
@@ Coverage Diff @@
## develop #4376 +/- ##
=============================================
- Coverage 69.12% 69.04% -0.09%
- Complexity 3654 3661 +7
=============================================
Files 266 266
Lines 15091 15123 +32
Branches 1638 1651 +13
=============================================
+ Hits 10432 10442 +10
- Misses 3896 3909 +13
- Partials 763 772 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
The unpredictability could be a pain, we'll have to keep an eye on this. |
| boolean success = false; | ||
| int maxRetries = 3; | ||
| int retries = 0; | ||
| do { |
There was a problem hiding this comment.
Neat, I think I had intended on implementing this at some point but never got around to it.
https://github.com/dockstore/dockstore/blob/1.11.5/dockstore-webservice/src/main/java/io/dockstore/webservice/resources/ResourceUtilities.java#L78
Might be useful as a general utility
| } | ||
|
|
||
| if (tokenResponse.statusCode() != HttpStatus.SC_OK) { | ||
| Map<String, List<Map<String, String>>> errorMap = GSON.fromJson(tokenResponse.body(), Map.class); |
There was a problem hiding this comment.
Can you always rely on it returning a JSON response for all error conditions? If not, I think this line will throw an exception.
It may be so rare that you don't need to handle it; your call.
There was a problem hiding this comment.
The Docker Registry API specs say that actionable failure conditions are reported as part of 4xx responses in a JSON response body, so I think it's pretty safe to assume that it'll return a JSON response
There was a problem hiding this comment.
In theory I would think you could get 5xx responses as well, which may not be actionable, and hence may not have have JSON responses.
I tend to think in edge cases, and this one is probably way out there, so what you have should be fine.
| if (manifestResponse.statusCode() == HttpStatus.SC_OK) { | ||
| success = true; | ||
| } else { | ||
| Map<String, List<Map<String, String>>> errorMap = GSON.fromJson(manifestResponse.body(), Map.class); |
| } | ||
|
|
||
| return gitHubContainerRegistryImages; | ||
| if (blobResponse.statusCode() != HttpStatus.SC_OK) { | ||
| Map<String, List<Map<String, String>>> errorMap = GSON.fromJson(blobResponse.body(), Map.class); |
There was a problem hiding this comment.
Same as other comments. Also just noticed that all 3 places where you do the error handling are near duplicates (I think), so you could create a common method.
|
SonarCloud Quality Gate failed. |
kathy-t
deleted the
feature/4330/amazon-ecr-workflow-checksum-harvesting
branch
For #4330
Uses the same checksum harvesting method as GitHub Container Registry images (idea 5 from the ticket).
Oddly enough, I ran into rate limit issues for the
GET manifestendpoint if I ran the Amazon ECR test around 5 times in a row, which doesn't happen with the GHCR test. I couldn't find any rate limit documentation for Amazon ECR's support of the Docker Registry API, and the responses don't have a rate limit header. Docker's is 100 per 6 hours for anonymous calls, but I don't think this is Amazon ECR's rate limit because I haven't had to wait 6 hours to re-run the tests. The rate limit issue was sporadic. The test would some times fail but then pass when I tried it again immediately.Got around the rate limit issue by implementing retries for the
GET manifestcall with exponentially back-off. In my testing, it only needed one retry to pass if there was a rate limit error.