perf(docker): Set cache data id, missing from un-grooving code #3961
Conversation
…pinnaker#3942) * perf(docker): Use parallel streams for caching docker images * chore(docker): Using java syntax for map updates * perf(docker): Only parallelize loop of tags * chore(docker): Use ConcurrentMap instead of ConcurrentHashMap * perf(docker): Fix for ConcurrentHashMap cast error. Added unit tests * perf(docker): Use parallel streams for caching docker images (cherry picked from commit 011da56)
|
Hi @ezimanyi -- this PR addresses the issue that was causing the previous PR to fail in our integration testing. It has been successfully tested in our environment and additional test coverage was added to ensure that the cache entry is stored with the correct key. |
| cacheResult.cacheResults.get("taggedImage").size() == total | ||
| cacheResult.cacheResults.get("imageId").size() == total | ||
| for (def i = 0; i < total; i++) { | ||
| cacheResult.cacheResults.get("taggedImage")[i].getId() != "" |
There was a problem hiding this comment.
It would probably make sense to ensure this is set to the expected value, not just that it's not the empty string. (This might involve stubbing the key generation logic if you want to keep it outside of the "unit" we're testing.)
| name="repo-1" | ||
| tags=["tag-1", "tag-2"] | ||
| } | ||
| client.getDigest(*_) >> "123" |
There was a problem hiding this comment.
It might be a bit more robust to return different digests for the different tags, and ensure the digests appear correctly in the output.
| then: | ||
| cacheResult.cacheResults.get("taggedImage").size() == total | ||
| cacheResult.cacheResults.get("imageId").size() == total | ||
| for (def i = 0; i < total; i++) { |
There was a problem hiding this comment.
The logic in this for loop is somewhat hard to follow, and also looks like it's only testing that each returned tag is something we have in repoTags but not the reverse---that everything in repoTags is returned in the result.
I'd suggest being more explicit about the expected results, and then asserting that the set of expected results is the same as the set of actual results.
|
@ezimanyi I improved tests validation by asserting on all expected exact values, they should be easier to read, and to troubleshoot failed tests on expected vs actual values. |
|
@spinnakerbot cherry-pick 1.15 |
* perf(docker): Fix for ConcurrentHashMap cast error. Added unit tests (#3942) * perf(docker): Use parallel streams for caching docker images * chore(docker): Using java syntax for map updates * perf(docker): Only parallelize loop of tags * chore(docker): Use ConcurrentMap instead of ConcurrentHashMap * perf(docker): Fix for ConcurrentHashMap cast error. Added unit tests * perf(docker): Use parallel streams for caching docker images (cherry picked from commit 011da56) * perf(docker): Set cache data id, missing from un-grooving code * perf(docker): Improved tests: explicit validations and extended coverage
|
Cherry pick successful: #3974 |
Changes of #3926 and #3942 (both reverted) adding the following:
DefaultCacheDataBuilderand include it in tests.This was previously set by groovy code implicitly rather than explicitly, by means of this code:
cachedTags was this
MapWithDefaultimplementation:Above code first tried to fetch an entry from the map with key
tagKey, and that caused the map to create and return a new instance ofDefaultCacheDataBuilderautomatically, with itsidfield set to the value oftagKey. When the code was ungroovied, id was missing to be set and cache entries were not properly saved, causing Igor to never find a new docker image.