Pass the artifacts checksums in the headers of the deploy request #199
Conversation
|
Hi @anancarv, could you please take a look at this? it seems like the token for codacy is missing |
|
Hi @anancarv just a friendly reminder that this MR is awaiting review |
There was a problem hiding this comment.
Hey @aslheyrr, I just reviewed your PR. But I don't get why you're not using this way of deploying checksums
| @@ -34,6 +34,7 @@ This library enables you to manage Artifactory resources such as users, groups, | |||
| + [Deploy an artifact](#deploy-an-artifact) | |||
| + [Deploy an artifact with properties](#deploy-an-artifact-with-properties) | |||
| + [Deploy an artifact with checksums](#deploy-an-artifact-with-checksums) | |||
| + [Deploy an artifact by checksums](#deploy-an-artifact-by-checksums) | |||
There was a problem hiding this comment.
I don't get why you want to create a new deploy by checksum instead of just editing the existing one. This would lead to confusion to have two ways to perform the same action
There was a problem hiding this comment.
I understand that there are two ways to deploy artifacts using checksums:
- Deploy by checksum: Based on the provided checksums, Artifactory checks if the artifact exists in Artifactory and if so, the artifact is virtually copied to the target location; otherwise no new files are deployed. Ref https://jfrog.com/help/r/jfrog-rest-apis/deploy-artifact-by-checksum
- Deploy with checksum (less documented than 1): the client deploys the checksums as header with the file (unlike 1, you can deploy new artifacts in Artifactory). Ref https://jfrog.com/help/r/what-are-client-checksum-server-checksum-and-checksum-policy-in-local-repositories/1.-verify-against-client-checksums-this-is-the-default-checksum-policy-for-local-repositories , and https://jfrog.com/help/r/artifactory-how-to-identify-and-fix-all-artifacts-with-missing-client-checksums/artifactory-how-to-identify-and-fix-all-artifacts-with-missing-client-checksums
As I see it, although both actions involve checksums, they both have different results that should be considered as separated methods.
I reckong this sounds a bit complicated, but I'll do my best to implement what is best for users.
There was a problem hiding this comment.
The first link you shared does not work.
Deploy with checksum (less documented than 1): the client deploys the checksums as header with the file (unlike 1, you can deploy new artifacts in Artifactory)
Ok, we can support it, but I think it's better to refactor the existing deploy method instead of creating a new function. That might confuse the users of the library. Maybe you can do something like this:
If checksum_enabled = False:
headers={
"X-Checksum-Deploy": "false",
"X-Checksum-Sha1": artifact_check_sums.sha1,
"X-Checksum-Sha256": artifact_check_sums.sha256,
"X-Checksum": artifact_check_sums.md5,
},
This way, you will deploy your checksums with the file. But what would happen if we deploy using method 2, then method 1 ("X-Checksum-Deploy": "true")?
pyartifactory/models/artifact.py
Outdated
| buf = fd.read(block_size) | ||
| while len(buf) > 0: | ||
| hasher.update(buf) | ||
| for algorithm in algorithms: |
There was a problem hiding this comment.
In my opinion, the old generate checksum works fine and does not need to be updated. Here you want to pass as input a list of algorithms, why that? What would happen if a user forget an algorithm (Sha256 for example) ? Will we deploy only 2 checksums ?
There was a problem hiding this comment.
I did it that way thinking of not everyone wants to be punished on performance for calculating all the checksums.
Checksums.generate is an internal class, in case a user forget an algoritm, the default ones will be used. You can see that at https://github.com/anancarv/python-artifactory/pull/199/files/cdd908f6a1dcd63114628f5beac86e654e6a88ea#diff-bd8c98fdc28a0b44ac12a172a61699adfe00b9254b02255b2b33b567ba2ba621R106
There was a problem hiding this comment.
I think it's preferable to keep it simple here. Also, I don't see use cases where people want to calculate only some checksums. Let's wait for more people to request that feature instead of unnecessarily complexifying the codebase
|
Hi @anancarv, did you see my comments? I'd like to get your feedback so we can move forward with this feature |
- Refactor the existing deploy method to push artifact's checksums
|
hey @anancarv I made some code changes in this branch, could you review them? |
Description
Artifactory UI shows a warning message in the uploaded artifacts when artifact checksums aren't provided, therefore we would like to push a checksum of binaries being deployed.
Pushing checksums doesn't seem to be a well documented feature in the JFrog documentation, however here is brief reference to it https://jfrog.com/help/r/why-am-i-getting-client-did-not-publish-a-checksum-value-for-npm-packages/why-am-i-getting-client-did-not-publish-a-checksum-value...-for-npm-packages
Type of change
Please delete options that are not relevant.
How has it been tested ?
Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration
Checklist: