-
Notifications
You must be signed in to change notification settings - Fork 554
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
ci(smoke-test): multi-arch container smoke test #11040
Conversation
38ff60b
to
75f243e
Compare
db5cebe
to
246fd3b
Compare
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.
👍 Generally looks good. Had some comments, nothing major.
qa/integration-tests/src/test/java/io/camunda/zeebe/it/smoke/ContainerClusterSmokeIT.java
Outdated
Show resolved
Hide resolved
qa/integration-tests/src/test/java/io/camunda/zeebe/it/smoke/ContainerClusterSmokeIT.java
Outdated
Show resolved
Hide resolved
qa/integration-tests/src/test/java/io/camunda/zeebe/it/smoke/ContainerSmokeTest.java
Outdated
Show resolved
Hide resolved
qa/integration-tests/src/test/java/io/camunda/zeebe/it/smoke/ContainerSmokeTest.java
Outdated
Show resolved
Hide resolved
qa/integration-tests/src/test/java/io/camunda/zeebe/it/smoke/ContainerClusterSmokeIT.java
Show resolved
Hide resolved
246fd3b
to
9fdc1ec
Compare
2cb7389
to
1350c24
Compare
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.
👍
Looks great! Some comments but nothing which needs another review
maven-extra-args: -T1C | ||
- uses: ./.github/actions/build-docker | ||
id: build-docker | ||
# Currently only Linux runners support building docker images without further ado |
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's rare to encounter a wild "ado" 😄 I guess it's technically appropriate it here though ;)
.github/workflows/ci.yml
Outdated
run: > | ||
mvn -B --no-snapshot-updates | ||
-DskipUTs -DskipChecks -Dsurefire.rerunFailingTestsCount=3 | ||
-pl qa/integration-tests | ||
-P smoke-test,extract-flaky-tests | ||
-Dsmoke.test.excludedGroups=$EXCLUDED_TEST_GROUPS |
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.
🔧 Surefire already has a property for this, e.g. excludedGroups
. So if you just set it to -DexcludedGroups
then you don't need to define an additional property. I guess it wouldn't play nice with profiles which may set additional excluded groups? I'm not 100% sure there, but it shouldn't be an issue here specifically.
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.
good point!
will actually go for the already available property, especially for this particular case "don't run container tests" I guess it would be nice if it also applies to different profiles, if we reuse that tag at some point.
If we hit issues with overrides we may address that then.
1350c24
to
b74fbdb
Compare
bors r+ |
Build succeeded: |
Description
Docker builds and smoke tests are only run on Linux machines as building docker containers on macOS and windows runners introduces complexity to the CI setup that we consider not worth the effort:
docker/build-push-action
windows/amd64
platform, see e.g. this logUsage of native arm64 runners is listed as a separate sub-task on #10986.
Related issues
closes #11020
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/1.3
) to the PR, in case that fails you need to create backports manually.Testing:
Documentation:
Please refer to our review guidelines.