ci: fix frequent nightly build failure in CI determining latest_edge_version #2298
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
agent-nodejs
💚 Build Succeeded
Expand to view the summary
Build stats
Trends 🧪 |
…loading index.tab Piping a curl download into commands that close the read pipe before curl has fully downloaded the file (per a Context-Length header) will result in curl erroring out. This breaks this script, because it is running with 'set -o pipefail' Fixes: #2296
trentm
changed the title
Member
Author
|
Discussion is all in #2296 |
Member
Author
|
Builds 5 and 7 at https://apm-ci.elastic.co/job/apm-agent-nodejs/job/apm-agent-nodejs-mbp/job/PR-2298/ should successful "egde" builds using the current PR changes. |
astorm
approved these changes
Aug 25, 2021
| set -x | ||
| export PS4='${BASH_SOURCE}:${LINENO}: ${FUNCNAME[0]:+${FUNCNAME[0]}(): }' | ||
| set -o xtrace | ||
|
|
Contributor
There was a problem hiding this comment.
I see here we're setting PS4 in order to change the -x/-xtrace prompt/prefix.
| index_tab_content=$(curl -sS "${NVM_NODEJS_ORG_MIRROR}/index.tab" \ | ||
| | (grep "^v${NODE_VERSION}" || true) | awk '{print $1}') | ||
| latest_edge_version=$(echo "$index_tab_content" | head -1) | ||
| if [[ -z "$latest_edge_version" ]]; then |
Contributor
There was a problem hiding this comment.
I see here we've broken up the previous one liner into two lines. I also am not sure why this second version works more consistently and the first version does not, but this does not appear to do any harm so 👍
Member
Author
There was a problem hiding this comment.
Details are in comments on #2296
- One of the reproducible issues with the former is that
curl | ... | head -1can mean (if the file being downloaded by curl is large enough) that the read pipe is closed byheadbeforecurlhas fully downloaded the file. Curl (with thecurl -soption we are using) errors out in this case; and because we are usingset -o pipefailin this bash script, the script exits non-zero. The solution there is to only include things in the pipeline that won't close the read pipe before all output is read. - The part that was still un-understood flailing for me is why downloading the full content to a local temp file and doing
cat "$localFile" | grep ... | awk ... | head -1would fail... but only in CI. I could not repro locally.
So the current solution is an unsatisfying answer that seems to work for now.
dgieselaar
pushed a commit
to dgieselaar/apm-agent-nodejs
that referenced
this pull request
Sep 10, 2021
…version (elastic#2298) Piping a curl download into commands that close the read pipe before curl has fully downloaded the file (per a Context-Length header) will result in curl erroring out with "Failed writing body". This breaks this script, because it is running with 'set -o pipefail'. An attempt to use a local temp file was discarded because of a not understood failure in CI. See elastic#2296 comments. Fixes: elastic#2296
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #2296