-
Notifications
You must be signed in to change notification settings - Fork 66
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migration to buildkite #135
Conversation
📚 Go benchmark reportDiff with the
report generated with https://pkg.go.dev/golang.org/x/perf/cmd/benchstat |
@cachedout while migrating this repo we discovered that benchmarks results are pushed to a central bucket. Are you using this results for any kind of automation? |
@@ -0,0 +1,60 @@ | |||
#!/usr/bin/env bash | |||
set -euxo pipefail |
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 script launch .ci/test.sh
, it looks like this script is doing more or less the same as that other file.
Or maybe, could we use that script directly as a command in the step definition ?
set +e | ||
list="$(go list ./... | grep -v /vendor/)" | ||
list_string="${list//$'\n'/ }" | ||
if [[ $root == "true" ]]; 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.
Where is it needed root
? In .ci/test.sh
, it looks like it is not needed root
.
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.
These tests are indeed running by non-root user in Jenkins, and the current builkite agent has user root inside the container, as a result, the different groups of tests will be running in these two cases. For making it the same, I changed a little bit the script (partially related to all comments)
- label: ":linux: Microbench" | ||
key: benchmark | ||
command: | ||
- ".buildkite/scripts/bench.sh" |
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.
Maybe we can keep the same shellscript under .ci
to be used in .buildkite.
In this case, here it could be directly used that file:
- ".buildkite/scripts/bench.sh" | |
- ".ci/bench.sh" |
WDYT ? @leo-ri @alexsapran
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.
Checking Jenkins pipeline, in that pipeline it is added comments in PRs for every pull request:
Line 68 in 3a9164f
notifyBuildResult(prComment: true, goBenchmarkComment: true) |
Should we add it now also in Buildkite ?
This is an example from elastic-package
https://github.com/elastic/elastic-package/blob/604c606c86bea611975fa9c9722a30a44b2204fb/catalog-info.yaml#L56-L57
@mrodm @alexsapran don't you think we can remove the Jenkinsfile at the same time here? |
This reverts commit 8ce7eac.
@leo-ri don't you think the |
discussed in person |
No description provided.