Skip to content
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

Merged
merged 3 commits into from
Nov 24, 2022

Conversation

megglos
Copy link
Contributor

@megglos megglos commented Nov 17, 2022

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:

  • macOS runners require the setup of colima as a docker runtime which adds another 1.5-2m to the runtime
  • on macOS testcontainers failed to detect the docker environment unless additional environment variables have been set
  • some docker related actions are not supported on windows runners like docker/build-push-action
  • the docker setup on Githubs windows runners seems to be bound to the windows/amd64 platform, see e.g. this log

Usage 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:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

@megglos megglos force-pushed the meg-11020-container-smoke-test branch 3 times, most recently from 38ff60b to 75f243e Compare November 17, 2022 17:26
@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2022

Test Results

   964 files  +    5     964 suites  +5   1h 52m 20s ⏱️ + 10m 41s
7 900 tests +398  7 893 ✔️ +398  7 💤 ±0  0 ±0 
8 106 runs  +404  8 097 ✔️ +404  9 💤 ±0  0 ±0 

Results for commit b74fbdb. ± Comparison against base commit 601c718.

♻️ This comment has been updated with latest results.

@megglos megglos force-pushed the meg-11020-container-smoke-test branch 22 times, most recently from db5cebe to 246fd3b Compare November 21, 2022 20:02
@megglos megglos marked this pull request as ready for review November 21, 2022 20:04
Copy link
Member

@npepinpe npepinpe left a 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.

@megglos megglos force-pushed the meg-11020-container-smoke-test branch from 246fd3b to 9fdc1ec Compare November 22, 2022 23:30
@megglos megglos force-pushed the meg-11020-container-smoke-test branch 2 times, most recently from 2cb7389 to 1350c24 Compare November 23, 2022 07:53
Copy link
Member

@npepinpe npepinpe left a 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
Copy link
Member

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 ;)

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
Copy link
Member

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.

Copy link
Contributor Author

@megglos megglos Nov 24, 2022

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.

@megglos megglos force-pushed the meg-11020-container-smoke-test branch from 1350c24 to b74fbdb Compare November 24, 2022 13:28
@megglos
Copy link
Contributor Author

megglos commented Nov 24, 2022

bors r+

@zeebe-bors-camunda
Copy link
Contributor

Build succeeded:

@zeebe-bors-camunda zeebe-bors-camunda bot merged commit 2017e37 into main Nov 24, 2022
@zeebe-bors-camunda zeebe-bors-camunda bot deleted the meg-11020-container-smoke-test branch November 24, 2022 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-arch smoke tests in CI
2 participants