Hide (confusing) curl errors in download-hz-dist outside of debug execution - #83
Conversation
Add conditional curl argument for showing errors in debug mode.
JackPGreen
changed the title
curl errors in download-hz-dist outside of debug execution
| --location | ||
| --output "${{ steps.derive-output-file.outputs.file }}" | ||
| --retry 5 | ||
| --retry-all-errors |
There was a problem hiding this comment.
This hides the noise, but most of it comes from --retry-all-errors - with --fail it retries 404s too, so every repo that doesn't have the artifact costs 5 retries (~31s) and 6 error lines. In the debug run the 3 misses took ~95s before snapshot-internal answered. 404 is the expected answer from n-1 repos here.
Shouldn't we drop --retry-all-errors instead? Plain --retry still retries timeouts and 408/429/5xx. WDYT?
There was a problem hiding this comment.
Shouldn't we drop
--retry-all-errorsinstead? Plain--retrystill retries timeouts and 408/429/5xx. WDYT?
I don't think so - I'm sure there are some cases of "non-retriable" errors where actually we do want to retry. It was GitHub, not JFrog/Maven, but intermittently returning 404 - #72 / https://github.com/orgs/community/discussions/53538
3 misses took ~95s before
Interesting - maybe a future optimisation could be to parallelise.
| --silent | ||
| ) | ||
|
|
||
| if [[ "${RUNNER_DEBUG:-}" == "1" ]]; then |
There was a problem hiding this comment.
With this, when all repos fail a regular run only logs Distribution not found in ... - a 401 from JFrog, a timeout and a real 404 look the same, and you need a debug re-run to tell them apart.
Couldn't we keep --show-error, capture stderr and print it only on failure? The RUNNER_DEBUG check goes away too - echodebug already hides it outside debug runs:
if curl_error=$(curl "${curl_args[@]}" --show-error "${url}" 2>&1); then
...
else
echodebug "Failed to download from ${url}: ${curl_error}"
errors+="${url}: ${curl_error}"$'\n'
fi
...
echoerr "Distribution not found:"$'\n'"${errors}"WDYT?
There was a problem hiding this comment.
Yes, this is a better solution - just more complex to implement. Initial approach was just a "quick win", but I've flipped to this instead.
Thinking aloud there might be an even more GitHub-centric solution with groups... but this will do.
8324639 to
4b69e22
Compare
|
| else | ||
| echodebug "Failed to download from ${url}" | ||
| echodebug "Failed to download from ${url}: ${curl_error}" | ||
| errors+="${url}: ${curl_error}"$'\n' |
There was a problem hiding this comment.
NIT
extra \n
errors+="${url}: $'\n${curl_error}"$'\n\n'
URLfeels long!
| done <<< "${{ steps.derive-repositories.outputs.repositories }}" | ||
|
|
||
| echoerr "Distribution not found in ${{ steps.derive-repositories.outputs.repositories }}" | ||
| echoerr "Distribution not found:"$'\n'"${errors}" |
There was a problem hiding this comment.
will end up printing twice when error and debug?
may be don't accumulate and just echodebug above and then say here like
echoerr "Distribution download failed. See errors above!"
or
accumulate and then echo the lot before exit
personally like to see the errors without debug!
There was a problem hiding this comment.
will end up printing twice when error and debug?
Debug prints command execution (i.e. set -x) so you'd get duplication anyway - just logging the calls passed to the echo functions in addition to what they do.
may be don't accumulate and just
echodebugabove and then say here like echoerr "Distribution download failed. See errors above!"
But @ldziedziul's point was for non-debug failure executions, we still want the logs, without re-executing. But not for successful. So we need to accumulate the logs.
When building Docker images, [we spend more time downloading the binaries than we do actually building the image](https://github.com/hazelcast/hazelcast-docker/actions/runs/35606429681/job/106354587693). Specifically, `retry-all-errors` makes sense when we expect the resource to be present - even if the server returns a `404`, if we _know_ the resource should be there, retrying is sensible. But in this case, we _don't_ know, and the "`404` but actually not really" scenario only applies to GitHub - not Maven central/JFrog. So retrying again and again before trying the next server _candidate_ on the list wastes time. By tweaking the retry condition to retry on more retry-appropriate errors, we can drop typical execution from [`1m 40s`](https://github.com/hazelcast/docker-actions/actions/runs/34609059099/job/103294644256#step:2:1) to [`4s`](https://github.com/hazelcast/docker-actions/actions/runs/35608547146/job/106361602018#step:2:1). Raised from [PR feedback](#83 (comment)).
3 participants
See Slack discussion - the action tries downloading from
nlocations, expecting (at most) one to contain the requested distribution. But printingn-1curl404errors to the logs in the case of success is confusing UX.Instead, lets only log those error messages in debug mode - when we might have the context (e.g. URL) to make any sense of them.
Example execution