Reduce api queries when getting latest release tag #1219
Conversation
metal3-io-bot
added
the
size/M
Sunnatillo
force-pushed
the
simplify-get-release-logic/sunnat
branch
from
f76ae56
to
b03a7af
Compare
|
/test-centos-integration-release-1-3 |
Sunnatillo
force-pushed
the
simplify-get-release-logic/sunnat
branch
4 times, most recently
from
b685f6e
to
0f7d491
Compare
|
/test-centos-integration-release-1-3 |
Sunnatillo
changed the title
Sunnatillo
force-pushed
the
simplify-get-release-logic/sunnat
branch
from
0f7d491
to
a30b0a9
Compare
Sunnatillo
force-pushed
the
simplify-get-release-logic/sunnat
branch
from
a30b0a9
to
b6a2354
Compare
Sunnatillo
force-pushed
the
simplify-get-release-logic/sunnat
branch
from
b6a2354
to
b83f687
Compare
Sunnatillo
force-pushed
the
simplify-get-release-logic/sunnat
branch
2 times, most recently
from
373e9c4
to
ed6e9b9
Compare
There was a problem hiding this comment.
Here you are removing the api pagination and keep checking only the last 100 releases created! so the function will fail if the target release is not in the first 100 batch ?
if the idea is just to limit the api request number you can increase the per_page to the max (i.e 100) while keeping the pagination since the current implementation will stop requesting the other pages if the target release found.
if you can guarantee that the target release will always be in the first api page with 100 elements you should make it clear in the function name for example :
function get_latest_release_from_first_githubapi_page() {
and add error message if the release was not found stating not found in the first page !
lib/releases.sh
Outdated
| if [ -z "${GITHUB_TOKEN:-}" ]; then | ||
| last_page="$(curl -s -I "${1}" | grep '^link:' | sed -e 's/^link:.*page=//g' -e 's/>.*$//g')" | ||
| release_tag=$(curl -sL "${url}" | jq .[].name -r | grep -E "${release}" -m 1) |
There was a problem hiding this comment.
Sorry for nitpicking this much, but please put .[].name in quotes for safer handling, and add the missing ^ in front of ${release} so it matches releases from the start only as it should.
| release_tag=$(curl -sL "${url}" | jq .[].name -r | grep -E "${release}" -m 1) | |
| release_tag=$(curl -sL "${url}" | jq ".[].name" -r | grep -E "^${release}" -m 1) |
CAPI only maintain two latest minor release of current api version, and latest minor release for older api version. We also follow CAPI. Considering newest releases will be always on top, I do not see necessity adding complications to function. IMHO, 100 latest release is good enough. I can add a note to function saying that it will check only latest 100 releases. |
Please note that although we no longer support v1alpha5 release we still use them (e.g in upgrade) and since there are no new patches they will keep going deep in bottom of the list as new releases are pushed on top not sure when but at some point they will get moved to the second page the proper behavior of this function is to go through the next pages if the release was not found on the first. I stand by keeping this behavior I remember when we hit this issue we increased the per_page to 100 as a workaround then we decided to implement proper api pagination use! We should not regress removing this implementation in the sake of limiting GitHub requests which we can do while keeping the same behavior. Check this issue : and previous PRs: |
Sunnatillo
force-pushed
the
simplify-get-release-logic/sunnat
branch
from
ed6e9b9
to
aca2c18
Compare
Sunnatillo
changed the title
|
/test-centos-integration-release-1-4 |
Sunnatillo
force-pushed
the
simplify-get-release-logic/sunnat
branch
from
aca2c18
to
aeee422
Compare
|
/test-centos-integration-release-1-4 |
|
/test-ubuntu-e2e-integration-main |
|
/test-ubuntu-e2e-integration-main Come on Cleura.... |
|
/test-ubuntu-e2e-integration-main |
1 similar comment
|
/test-ubuntu-e2e-integration-main |
lib/releases.sh
Outdated
| if [ -z "${GITHUB_TOKEN:-}" ]; then | ||
| response=$(curl -si "${url}") | ||
| else | ||
| response=$(curl -si "${url}" -H "Authorization: token ${GITHUB_TOKEN}") | ||
| fi | ||
|
|
||
| done | ||
| # Divide response to headers and body | ||
| response_headers=$(echo "${response}" | awk 'BEGIN {RS="\r\n\r\n"} NR==1 {print}') | ||
| response_body=$(echo "${response}" | awk 'BEGIN {RS="\r\n\r\n"} NR==2 {print}') | ||
|
|
||
| # get the last page of releases from headers | ||
| last_page=$(echo "${response_headers}" | grep '^link:' | sed -e 's/^link:.*page=//g' -e 's/>.*$//g') |
There was a problem hiding this comment.
something is printing the full response on the logs and you know how huge it is !
There was a problem hiding this comment.
it has increased the logs size from around 482 KB to 73,059 KB and Jenkins is struggling show full logs 💥
There was a problem hiding this comment.
/hold
until logs are silenced.
Thank you for noticing it.
metal3-io-bot
added
the
do-not-merge/hold
|
/hold |
Sunnatillo
force-pushed
the
simplify-get-release-logic/sunnat
branch
from
aeee422
to
1254f6c
Compare
|
/test-centos-integration-release-1-4 |
|
/test-ubuntu-e2e-integration-main |
|
/unhold |
metal3-io-bot
removed
the
do-not-merge/hold
|
/lgtm |
metal3-io-bot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kashifest, lentzi90, tuminoid The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR reduces get_latest_release functions queries to github api for getting latest release tags.