-
Notifications
You must be signed in to change notification settings - Fork 90
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
Integration for Buildkite #4294
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
This reverts commit fa0c02f.
make build-webui | ||
make test-and-report |
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.
do we need to build the webui with each test?
@@ -0,0 +1,43 @@ | |||
FROM buildkite/hosted-agent-base |
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.
Nice. Much cleaner that circleci setup. Can we put everything related to buildkite under /buidlkite
directory, including this Dockerfile?
cache: | ||
paths: | ||
- "/golangci-lint-cache" | ||
- "/vendor-cache" | ||
- "/node-modules-cache" | ||
size: 20g | ||
name: "vendor-module-cache" |
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.
how is this cache used? We also want to make sure we build once, and then use the builds in later phases, such as running unit tests, integration tests, and releasing them if this build includes a release
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.
also do we pull the code once, or every step in the pipeline pulls the code again?
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.
Every step in pipeline unfortunately pulls code :( But we use git mirroring which fastens up.
If you want to do just once. You cant take advantage of parallel builds.
Circle CI was the same too.
- group: ":package: Build Tarball" | ||
key: "build-tarball" | ||
steps: | ||
- label: ":golang: Build linux amd64" | ||
command: "./buildkite/scripts/build_tarball.sh linux amd64" | ||
agents: | ||
queue: "buildkite-hosted-linux-small" | ||
|
||
- label: ":golang: Build linux arm64" | ||
command: "./buildkite/scripts/build_tarball.sh linux arm64" | ||
agents: | ||
queue: "buildkite-hosted-linux-small" |
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.
how are steps in the same group executed? Do each require spinning up their own container, which means slower start and cost, or are they run within the same agent/container? I am double checking because we define the agents in each step, which implies they run on separate environments
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.
They are on different agents. Spinning up the agent is pretty fast. It stays in queue for max 5-6s. Group step only helps seeing things visually !
- label: ":testengine: Unit Test" | ||
command: "./buildkite/scripts/test.sh unit" | ||
key: "unit-test" | ||
agents: | ||
queue: "buildkite-hosted-linux-large" | ||
|
||
- label: ":testengine: Integration Test" | ||
command: "./buildkite/scripts/test.sh integration" | ||
key: "integration-test" | ||
agents: | ||
queue: "buildkite-hosted-linux-large" |
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.
eventually we want to run on different environments, including windows, linux arm, linux amd, and darwin.
Also, separating unit and integration tests into separate steps was done before because of consistent failures in integration tests and to be able to retry only the failed steps in circleci instead of the whole tests again. We can continue to do the same here as well if it makes sense, but we should re-evaluate if this is adding unnecessary cost
buildkite/scripts/build_tarball.sh
Outdated
#!/bin/bash | ||
|
||
GOOS=$1 GOARCH=$2 make build-bacalhau-tgz |
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 looked at the build logs https://buildkite.com/expanso/bacalhau-golang/builds/291#01910786-63c9-4a7e-b38d-d12c63b50496
some observations:
- It seems we are building the webui everytime. This is an issue with make target setup
- I see some failures at the end related to signing the builds because of missing /tmp/private.pem. Check
Line 317 in e71b30b
name: Build tarball
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.
Yess ! I want to have at least one green build with buildkite before I merge this. I am debugging those.
This reverts commit 4e5d92b.
@@ -381,6 +384,7 @@ func (s *ExecutorTestSuite) TestDockerNetworkingFiltersHTTP() { | |||
} | |||
|
|||
func (s *ExecutorTestSuite) TestDockerNetworkingFiltersHTTPS() { | |||
s.T().Skip("Skipping the test until buildkite is fixed.") |
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.
nit: will be nice to only skip in buildkite and let them run when testing locally. Env variables can help with that
# Apply rate limits to the outbound connections. We just do this for all | ||
# interfaces rather than working out which is our Internet connection. | ||
while IFS= read -r IFACE; do | ||
tc qdisc add dev "${IFACE}" root tbf rate 10mbit burst 32kbit latency 10sec | ||
done < <(ip --json address show | jq -rc '.[] | .ifname') | ||
|
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.
do we need to remove these even if we are skipping the tests? If yes, then please add an issue to make sure we fix this way before 1.5
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.
Done. We don't need to remove this. So reverted it.
This PR aims at the following
closes #4297