Return checksum information #7
Conversation
github-actions
bot
requested review from
eddumelendez,
helpermethod and
marc0der
There was a problem hiding this comment.
I'd suggest we first take a few steps back and think/discuss this work a bit more. We are affecting changes on the backend without having considered the frontend (CLI).
We should be driving all changes from the CLI, which will then naturally lead us to implement stuff on the backend. In other words, from the outside in. It feels like we're jumping the gun a little by making assumptions in the backend about what the frontend will need before even having written the code. Again, let's first talk on Slack.
| @@ -26,6 +26,17 @@ Feature: Download a Candidate Version | |||
| Then a redirect to "http://dl.bintray.com/groovy/maven/apache-groovy-binary-2.4.7.zip" is returned | |||
| And an audit entry for groovy 2.4.7 UNIVERSAL is recorded for LinuxX64 | |||
|
|
|||
| Scenario: Retrieve checksum when not found | |||
There was a problem hiding this comment.
I think checksum might be a feature, so should probably live in a feature file of it's own.
| @@ -26,6 +26,17 @@ Feature: Download a Candidate Version | |||
| Then a redirect to "http://dl.bintray.com/groovy/maven/apache-groovy-binary-2.4.7.zip" is returned | |||
| And an audit entry for groovy 2.4.7 UNIVERSAL is recorded for LinuxX64 | |||
|
|
|||
| Scenario: Retrieve checksum when not found | |||
| Given a valid UNIVERSAL binary for groovy 2.4.7 hosted at http://dl.bintray.com/groovy/maven/apache-groovy-binary-2.4.7.zip | |||
| When a download request is made on "/download/groovy/2.4.7/linux?checksum" | |||
There was a problem hiding this comment.
I'm not convinced about the query param. Mind if we discuss this a bit more?
| Given a valid UNIVERSAL binary for groovy 2.4.7 from http://dl.bintray.com/groovy/maven/apache-groovy-binary-2.4.7.zip with algorithm SHA-512 and checksum groovy2.4.7-hash | ||
| When a download request is made on "/download/groovy/2.4.7/linux?checksum" | ||
| Then the service response status is 200 | ||
| And the response is "SHA-512|groovy2.4.7-hash" |
There was a problem hiding this comment.
I'd expect to see a real example here. How about using a real hash?
| @@ -16,6 +16,11 @@ And(~/^a valid (.*) binary for (.*) (.*) hosted at (.*)$/) { String platform, St | |||
| insertCandidateVersionInDb(db, candidate, version, platform, url) | |||
| } | |||
|
|
|||
| And(~/^a valid (.*) binary for (.*) (.*) from (.*) with algorithm (.*) and checksum (.*)$/) { | |||
| String platform, String candidate, String version, String url, String algorithm, String checksum -> | |||
| insertCandidateVersionInDb(db, candidate, version, platform, url, algorithm, checksum) | |||
There was a problem hiding this comment.
Let's talk a bit more about how we should persist this. Please see my comments on the DB migration PR.
| @@ -62,6 +67,8 @@ class MongoHelper { | |||
| basicDbObject.append("version", version) | |||
| basicDbObject.append("platform", platform) | |||
| basicDbObject.append("url", target) | |||
| if(algorithm != null) basicDbObject.append("algorithm", algorithm) | |||
There was a problem hiding this comment.
You can use the Groovy truth here:
| if(algorithm != null) basicDbObject.append("algorithm", algorithm) | |
| if(algorithm) basicDbObject.append("algorithm", algorithm) |
| ctx.redirect(302, v.getUrl()); | ||
| audit(details, p.id(), v.getPlatform()); | ||
|
|
||
| if (!details.isRenderChecksum()) { |
There was a problem hiding this comment.
It feels wrong to be adding this to the CandidateDownloadHandler. The single responsibility of this handler is to provide candidate downloads. I think we should have an entirely new Handler class for checksums.
Add
?checksumquery parameter to return the artifact's checksum info (see #6)