- Notifications
You must be signed in to change notification settings - Fork928
chore: reduce "Upload tests to datadog" times in CI#17668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
bb80dee to5d42295Compare| exit 1 | ||
| fi | ||
| echo "Downloading DataDog CI binary version ${BINARY_VERSION} for ${{ runner.os }}..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Needs SHA256 validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Which will be hard now that the version is an input...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll remove the input then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why would it need SHA256 validation? We are downloading from the official repo, and also, the CLI only works if the correct token is available? Also, the test logs are all public; there is no sensitive data. Or. is it, and I am missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It mitigates supply-chain attacks. If somebody took over Datadog's repo, they'd be able to swap the release files for a malicious payload. AFAIK, release artifacts are mutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@deansheather I added hash verification
5d42295 tof589664Compare| BINARY_HASH_MACOS="e87c808638fddb21a87a5c4584b68ba802965eb0a593d43959c81f67246bd9eb" | ||
| BINARY_HASH_LINUX="5e700c465728fff8313e77c2d5ba1ce19a736168735137e1ddc7c6346ed48208" | ||
| TMP_DIR=$(mktemp -d) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could this go in runner temp instead so it hits the ramdisk? The perf savings are probably miniscule but might as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The perf savings are probably miniscule
Indeed, I don't think it's worth optimizing further if the entire step already takes only 3 seconds
6b4d3f8 intomainUh oh!
There was an error while loading.Please reload this page.
For future reference, this helps unblock#15109. |
Uh oh!
There was an error while loading.Please reload this page.
This PR speeds up the "Upload tests to datadog" step by downloading the
datadog-cibinary directly from GitHub releases. Most of the time used to be spent innpm install, which consistently timed out on Windows after a minute.Now it takes 3 seconds.I updated it to version v2.48.0 because v2.21.0 didn't have the artifacts for arm64 macOS.