fix: refactor replace_image_pullspec to use other helper functions #361
Conversation
a recent commit introduced helper functions to retrieve repo, tag and digest of an image. this commit refactors replace_image_pullspec to use the helper function instead of implementing the logic inside the function. Signed-off-by: Yashvardhan Nanavati <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #361 +/- ##
========================================
Coverage ? 100.00%
========================================
Files ? 18
Lines ? 502
Branches ? 0
========================================
Hits ? 502
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
@arewm Could you please help review this? |
| if ! [[ "$image" =~ $image_regex ]]; then | ||
| echo "replace_image_pullspec: invalid pullspec format: ${image}" >&2 | ||
| exit 2 | ||
| fi |
There was a problem hiding this comment.
Do we need this or can we just rely on the validation of parse_image_url? Do we need to improve the validation of that function?
There was a problem hiding this comment.
I tried relying on the validation in parse_image_url but one of the tests that verifies the sha is of the required length fails.
Enhancing the validation to that function causes nearly 20 other tests to fail mainly because dummy sha's are used in the unit tests.
I can fix the validation and tests in parse_image_url and update the PR
There was a problem hiding this comment.
Ah. Given the fact that we have encountered multiple bugs due to test cases not accurately reflecting the real world, while this is a non-ideal case, I think the change might be beneficial in the long run.
There was a problem hiding this comment.
I agree with @arewm, if the test cases don't reflect the real world, they should be updated.
| tag=$(echo "$parsed_image" | jq -r '.tag // empty') | ||
| digest=$(echo "$parsed_image" | jq -r '.digest // empty') | ||
|
|
||
| local replaced_pullspec="$mirror" | ||
| if [ -n "$tag" ]; then | ||
| replaced_pullspec+=":${tag}" | ||
| fi | ||
|
|
||
| if [ -n "$digest" ]; then | ||
| replaced_pullspec+="@${digest}" | ||
| fi |
There was a problem hiding this comment.
We already effectively do this in the helper functions when returning the tag and digest. With that, we check to see if the tag and digest exist before we append it to the image.
What if we add one more helper function to just return the tag and digest (checking for their existence) and then append that to the mirror?
If you don't want to do that, then we can also just reuse the same jq query to append the tag and digest (if they exist) to the mirror.
Line 259 in f9a0dbb
dirgim
requested review from
14rcole,
Josh-Everett,
dirgim,
hongweiliu17,
kasemAlem,
dheerajodha,
sonam1412,
jsztuka,
jencull and
nmars
a recent commit introduced helper functions to retrieve repo, tag and digest of an image. this commit refactors replace_image_pullspec to use the helper function instead of implementing the logic inside the function.