fix: use redirect URL to fetch latest Dolt version#10628
fix: use redirect URL to fetch latest Dolt version#10628nullun wants to merge 7 commits intodolthub:mainfrom
Conversation
Docker expands the `DOLT_VERSION` ARG before bash execution. When set to `latest`, the dynamically fetched version number was being overwritten, causing the script to request `/vlatest/install.sh` and fail with a 404. This commit introduces an internal `ACTUAL_VERSION` variable in the bash script to bypass Docker's string substitution.
This reverts commit 29dba59.
|
Just realised I should probably remove the |
…nload" This reverts commit 2c41c8e.
41ba3c3 to
b020044
Compare
The GitHub API was rate limiting unauthenticated requests in CI, causing the version fetch to silently return an empty string and the subsequent install script download to fail.
b020044 to
0aeaac1
Compare
|
Well, that was a journey... Sorry for performing so many tests. My initial attempt didn't fix it, but after avoiding the API rate limit it appears to have solved the issue. On reflection I do still think my first commit fixes an issue, as one of the logs showed |
d0c86d2 to
60d06f8
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a flaky CI test (docker-entrypoint: latest binary build from dolt directory) by switching from the GitHub REST API (/repos/dolthub/dolt/releases/latest JSON endpoint) to a redirect-URL approach (curl -sI on github.com/dolthub/dolt/releases/latest) for fetching the latest Dolt version. Unauthenticated GitHub API calls in CI are subject to rate limiting, which caused the version to be silently empty and the build to fail with a confusing Not: command not found error. The test's skip statement that was added as a workaround is also removed.
Changes:
docker/serverDockerfile: Replaces GitHub API-based version lookup with HTTP HEAD redirect-based lookup; introducesACTUAL_VERSIONshell variable to avoid shadowing the DockerARG DOLT_VERSION.integration-tests/bats/docker-entrypoint.bats: Removes theskipon the previously flaky test and updatesEXPECTED_VERSIONto use the same redirect-based approach.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
docker/serverDockerfile |
Fetches latest Dolt version via redirect URL instead of GitHub API to avoid rate limiting; introduces ACTUAL_VERSION variable |
integration-tests/bats/docker-entrypoint.bats |
Removes skip, aligns EXPECTED_VERSION fetch with the Dockerfile's new approach |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
i'm still thinking about this... I don't love that we fetch the latest binary from github at all in this test, where I think the intention is to demonstrate that the server dockerfile on this branch has not had a regression. @elianddb can maybe chime in here. I think to fix this we should make building the docker image successfully one test, and we should build it from the source so its not making and http request, and if the build exits with 0, it passes. then in subesquent tests we can use that built image as a dependency, so if it exists, run tests using it, or if it doesnt exist, skip the tests. something like that... |
|
|
||
| LATEST_IMAGE="dolt-entrypoint-latest:test" | ||
| EXPECTED_VERSION=$(curl -s https://api.github.com/repos/dolthub/dolt/releases/latest | grep '"tag_name"' | cut -d'"' -f4 | sed 's/^v//') | ||
| EXPECTED_VERSION=$(curl -sI https://github.com/dolthub/dolt/releases/latest | grep -i location | sed 's|.*/v||' | tr -d '[:space:]') |
There was a problem hiding this comment.
I've spoken with @coffeegoddd and the main issue with this is we're relying on an external call to github. Our best bet would be to check if the docker code contains this line itself. This also goes for the test below.
|
@nullun I think it would be best so we don't rely on external Github calls to check for the existence of the specific links in the Dockerfile. I do think it's fine to update to not use the api link version tho, if it helps us avoid rate limiting. |
… vlatest" This reverts commit 60d06f8.
I saw my PR (#10591) failing to this test.
I believe docker is expanding theDOLT_VERSIONARG before bash execution. So when set tolatest, the dynamically fetched version number was being overwritten, causing the script to try and download/vlatest/install.shand fails.This commit introduces an internalACTUAL_VERSIONvariable in the bash script to bypass Docker's string substitution.The
download-binarystage fetches the latest Dolt version from the GitHub API. In CI, unauthenticated requests to the GitHub API can be rate limited, causing the request to fail silently. The version variable ends up empty, the install script URL is malformed, and the build fails with a confusingNot: command not founderror.This is a pre-existing issue unrelated to my other changes.
Hopefully this solves the mystery of the flakey test #10625