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.

Sign up for GitHub

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

Jump to bottom

[ci] Move cpp-tests job inside Pulsar CI workflow #17571

Merged
merged 8 commits into from
Sep 13, 2022
Merged

[ci] Move cpp-tests job inside Pulsar CI workflow #17571

merged 8 commits into from
Sep 13, 2022

Conversation

nicoloboschi
Copy link
Contributor

Motivation

There are multiple benefits to adding the cpp-tests inside the main workflow:

  • Now CPP tests runner is triggered even for docs-only changes
  • The build artifacts are not reused
  • CPP tests can be skipped in case of integration-tests failures or build broken or checkstyle errors etc.
  • Less workflows

Modifications

  • Moved the job right after the integration-tests
  • Now maven artifacts are reused from the previous job
  • Added dependency to the new job for the latest step Pulsar CI checks completed
  • Removed cpp-tests from the required checks because now it's included in Pulsar CI checks completed
  • doc-not-needed

@nicoloboschi nicoloboschi requested a review from lhotari September 9, 2022 09:29
lhotari
lhotari requested changes Sep 9, 2022
View reviewed changes
.github/workflows/pulsar-ci.yaml Outdated Show resolved Hide resolved
.github/workflows/pulsar-ci.yaml Outdated Show resolved Hide resolved
lhotari
lhotari requested changes Sep 9, 2022
View reviewed changes
.github/workflows/pulsar-ci.yaml Outdated Show resolved Hide resolved
lhotari
lhotari approved these changes Sep 9, 2022
View reviewed changes
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 9, 2022
tisonkun
tisonkun reviewed Sep 9, 2022
View reviewed changes
.asf.yaml Show resolved Hide resolved
@tisonkun
Copy link
Member

tisonkun commented Sep 9, 2022

BTW, this PR triggers only part of CI tasks. I may assume that a change in pulsar-ci.yml should trigger almost full test cases?

@tisonkun
Copy link
Member

tisonkun commented Sep 9, 2022

It seems that we still take a long time to complete CI. For example, 10f7209 made 11 hours ago and now it's not completed.

@nicoloboschi nicoloboschi force-pushed the ci-include-cpp-tests-in-pulsarci branch from c2bb34a to 77169ff Compare September 12, 2022 07:15
@nicoloboschi nicoloboschi force-pushed the ci-include-cpp-tests-in-pulsarci branch from 77169ff to 7bc2ab7 Compare September 12, 2022 13:29
lhotari
lhotari requested changes Sep 12, 2022
View reviewed changes
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to add a name for the build job so that it looks consistent in the UI.

.github/workflows/pulsar-ci.yaml Show resolved Hide resolved
tisonkun
tisonkun approved these changes Sep 12, 2022
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@nicoloboschi nicoloboschi merged commit 1f50366 into apache:master Sep 13, 2022
@nicoloboschi nicoloboschi deleted the ci-include-cpp-tests-in-pulsarci branch September 13, 2022 09:37
@lhotari lhotari mentioned this pull request Sep 13, 2022
Merged
4 tasks
tisonkun pushed a commit to tisonkun/pulsar that referenced this pull request Sep 14, 2022
@nicoloboschi @lhotari @tisonkun
[ci] Move cpp-tests job inside Pulsar CI workflow (apache#17571)
6888484 
Co-authored-by: Lari Hotari <[email protected]>
@tisonkun tisonkun mentioned this pull request Sep 20, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shibd shibd shibd approved these changes

@Demogorgon314 Demogorgon314 Demogorgon314 approved these changes

@tisonkun tisonkun tisonkun approved these changes

@nodece nodece nodece approved these changes

@lhotari lhotari lhotari approved these changes

Assignees
No one assigned
Labels
area/ci doc-not-needed Your PR changes do not impact docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nicoloboschi @tisonkun @lhotari @nodece @Demogorgon314 @shibd