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

ref(ci): Use a single CI workflow for tests #8575

Merged
merged 8 commits into from
Jun 10, 2024
Merged

Conversation

gustavovalverde
Copy link
Member

@gustavovalverde gustavovalverde commented Jun 3, 2024

Motivation

This is an initial implementation to solve the re-building of our Docker image, which is being built multiple times by our Github Actions, causing elevated costs across multiple services.

Fixes #7816

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Complex Code or Requirements

  • This required creating an extra workflow ci-tests.yml with its respectives .patch.yml and .patch-external.yml to initialize the unit tests and integration tests from a single place, effectively using a single Docker image build process.
  • This must be admin merged, and all open PRs must be rebased afterwards.

Solution

  • Create a Run tests starter workflow
  • Convert the Integration Tests on GCP and Docker Unit Tests to callable workflows
  • Merge both of the Integration Tests on GCP and Docker Unit Tests patches files into a single patch file for Run tests

Testing

  • Tests should run on this PR successfully

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

This must be followed by #8374 to effectively reduce our costs.

@gustavovalverde gustavovalverde added C-bug Category: This is a bug C-design Category: Software design work A-devops Area: Pipelines, CI/CD and Dockerfiles I-cost Zebra infrastructure costs P-High 🔥 labels Jun 3, 2024
@gustavovalverde gustavovalverde self-assigned this Jun 3, 2024
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jun 3, 2024
@gustavovalverde gustavovalverde marked this pull request as ready for review June 4, 2024 02:16
@gustavovalverde gustavovalverde requested a review from a team as a code owner June 4, 2024 02:16
@gustavovalverde gustavovalverde requested review from arya2 and removed request for a team June 4, 2024 02:16
This is an initial implementation to solve the re-building of our Docker image, which is being built multiple times by our Github Actions.

This is meant to fix -> devops: Build CI Docker runs twice for every PR #7816
@gustavovalverde gustavovalverde added the extra-reviews This PR needs at least 2 reviews to merge label Jun 7, 2024
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

This PR removes workflow patches for unit tests, but it doesn't do so for integration tests. Why is that?

.github/workflows/ci-tests.yml Show resolved Hide resolved
.github/workflows/ci-tests.yml Outdated Show resolved Hide resolved
.github/workflows/ci-tests.yml Outdated Show resolved Hide resolved
.github/workflows/ci-tests.patch-external.yml Show resolved Hide resolved
@gustavovalverde
Copy link
Member Author

This PR removes workflow patches for unit tests, but it doesn't do so for integration tests. Why is that?

The changelog is confusing, but I removed the unit tests patches, and renamed and reused the same patch files for integration tests, those are the new ci-tests.patch.yml and ci-tests.patch-external.yml. This was mainly to reduce the diff.

upbqdn
upbqdn previously approved these changes Jun 10, 2024
.github/workflows/ci-tests.patch-external.yml Outdated Show resolved Hide resolved
.github/workflows/ci-tests.patch.yml Outdated Show resolved Hide resolved
@gustavovalverde
Copy link
Member Author

@upbqdn after merging your last suggestions, this will require re-approval. Thanks!

upbqdn
upbqdn previously approved these changes Jun 10, 2024
@gustavovalverde
Copy link
Member Author

@upbqdn Oops! I had to revert one of the suggestions as I had forgotten the reasoning behind it.

The job is called Build images as the specific workflow that's being (./.github/workflows/sub-build-docker-image.yml) called is used for CI, CD and Release images.

Also, changing the job name here would then require 2 merge rules instead of 1:

Now:

  • Build CI Docker / Build images and Build CD Docker / Build images (1 rule)
    After:
  • Build CI Docker / Build CI image (1 rule)
  • Build CD Docker / Build CD image (1 rule)

This would also be a breaking change, requiring other PRs to be updated. I'd prefer to avoid that in this refactor.

upbqdn
upbqdn previously approved these changes Jun 10, 2024
.github/workflows/ci-tests.patch.yml Outdated Show resolved Hide resolved
@gustavovalverde gustavovalverde removed the extra-reviews This PR needs at least 2 reviews to merge label Jun 10, 2024
mergify bot added a commit that referenced this pull request Jun 10, 2024
@mergify mergify bot merged commit 55c6992 into main Jun 10, 2024
148 checks passed
@mergify mergify bot deleted the fix-duplicate-builds branch June 10, 2024 22:51
@upbqdn upbqdn mentioned this pull request Jun 28, 2024
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-bug Category: This is a bug C-design Category: Software design work C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-cost Zebra infrastructure costs P-High 🔥
Projects
None yet
Development

Successfully merging this pull request may close these issues.

devops: Build CI Docker runs twice for every PR
2 participants