For #110: Added meaningful error for broken package in Rpm.batchUpdate #210
Conversation
Codecov Report
@@ Coverage Diff @@
## master #210 +/- ##
=========================================
Coverage 86.40% 86.40%
Complexity 269 269
=========================================
Files 40 40
Lines 1258 1258
Branches 55 55
=========================================
Hits 1087 1087
Misses 160 160
Partials 11 11 Continue to review full report at Codecov.
|
|
This pull request #210 is assigned to @carlosmiranda/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @g4s8/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be a monetary reward for this job |
| Assertions.assertThrows( | ||
| IllegalArgumentException.class, | ||
| () -> repo.batchUpdate(Key.ROOT).blockingAwait(), | ||
| "RPM package \"brokentwo.rpm\" is broken, can't be read" |
There was a problem hiding this comment.
@paulodamaso let's rephrase this a bit:
Reading of RPM package failed, data corrupt or malformed.
|
@carlosmiranda Done, please take a look |
There was a problem hiding this comment.
@paulodamaso looks good, just need to sync with master. @g4s8 everything good on my end
| ); | ||
| repo.batchUpdate(Key.ROOT).blockingAwait(); | ||
| final byte[] broken = {0x00, 0x01, 0x02 }; | ||
| storage.save( |
There was a problem hiding this comment.
@paulodamaso it's async call which returns CompletableFuture, you either need to wait it with .toCompletableFuture().get() or wrap this call with BlockingStorage
|
@carlosmiranda/z this job was assigned to you 5days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please. |
|
@g4s8 Corrections made, please take a look |
|
@g4s8 ping |
There was a problem hiding this comment.
@paulodamaso added several comments, take a look please. Also I do not see any puzzles here...
| storage.save( | ||
| new Key.From("stored.rpm"), | ||
| new Content.From("stored content".getBytes()) | ||
| ).toCompletableFuture().get(); |
There was a problem hiding this comment.
@paulodamaso please use putFilesInStorage to add some real rpms into storage
| new Content.From( | ||
| broken | ||
| ) | ||
| ); |
There was a problem hiding this comment.
@paulodamaso please call join here, nothing was actually added to storage
| broken | ||
| ) | ||
| ); | ||
| Assertions.assertThrows( |
There was a problem hiding this comment.
@paulodamaso to check that exception was thrown is not enough: we need to be sure that metadata files were not changed: I'd suggest obtaining information about metadatafiles after the first update and use it here to compare agains metadata after the second update.
|
@olenagerasimova Done, please take a look. Please note that this PR is about checking that the exception was not thrown; we have another issue to assure that we didn't touched the metadata when invalid packages are updated (#190) |
|
@paulodamaso thanks for the corrections and comment, I want to discuss one moment though: as you correctly noticed, #190 (closed already) and #110 have some behavior conflicts. In #110 we decided to that failing the update is more correct, which means, that #110 is not finished yet, but I do not see any puzzles in this PR. Please, could you add the puzzle and explain what we should fail the update and do not touch metadata if invalid package was met? |
|
@olenagerasimova Done, please take a look |
There was a problem hiding this comment.
@paulodamaso thanks, looks good to me
@g4s8 please approve/merge
|
@paulodamaso please resolve conflict |
|
@paulodamaso please resolve conflicts |
|
@olenagerasimova Done, please merge |
� Conflicts: � src/test/java/com/artipie/rpm/RpmTest.java
|
@g4s8 please approve and I'll merge |
|
@sereshqua/z please review this job completed by @carlosmiranda/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
Code review was too long (17 days), architects (@g4s8, @olenagerasimova/z) were penalized, see §55 |
|
@carlosmiranda please make sure you will find at least 3 issues during next CR, thanks |
|
@0crat quality acceptable |
|
@sereshqua confirmed, sorry for the delay. I suffered a laptop crash last week. |
|
@carlosmiranda np :) |
For #110:
Rpm.batchUpdateis throwing an exception when it has a broken package.@g4s8 Notice that the behavior somewhat conflicts with #190 : if we are expecting for an exception to be thrown, the metadata content won't be changed after all. Also seems that we should add packages through the
Rpminterface instead adding them directly to storage, and callbatchUpdateinternally