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 enhancements: Code linter, PR description, speed up #261

Merged
merged 8 commits into from
Nov 4, 2024

Conversation

sirknightj
Copy link
Contributor

@sirknightj sirknightj commented Nov 1, 2024

Issue #, if available:
N/A

What was changed?

Summary of Changes: This pull request introduces several enhancements to the Continuous Integration (CI) pipeline, focusing on efficiency, speed, and developer experience.

Detailed Changes:

  1. Refactor the CI to use DRY principle.
  • Implemented matrix strategy to reduce redundancy in generating different build combinations.
  1. Speed up the builds and tests
  • Leveraged make's parallel feature to accelerate build processes.
  • Utilized matrix strategy to optimize test case execution, eliminating redundant builds and runs per instance.
  • Previously, each instance would build one combination. Run it. Delete everything. Then build a different combination. Then run it. Resulting in 2 builds and runs per instance.

Performance improvement:
Before (note some of the jobs fail in under 30s, those are due to failures):
image

After:

  • Some jobs (like the arm-32-cross-compilation) we see a 50% reduction in time and 65% for arm-64)
image
  1. Fix the GitHub description linter to allow for * to be used in the PR description.
  1. Linter Job Enhancements
  • Linter now checks tst/ directory (in addition to the current src/ directory)
  • Note: Some of the fields in the configuration were adjusted. Some fields don't accept boolean values and I adjusted those fields to use the equivalent value.
  • Improved error reporting to provide specific file locations and correction suggestions.
  • Implemented success/failure summaries in run reports.
  • Added functionality to generate and attach patch files for required formatting changes.

If it passes, you will see the success message in the run summary.
image

If it fails, you will see a list of files that are not formatted correctly.
image
image

Attached to the summary, a patch file is included that you can apply the changes. It also uploads the full corrected file(s) that need formatting.

You can check the example here: https://github.com/awslabs/amazon-kinesis-video-streams-pic/actions/runs/11624707897

Why was it changed?

  • Current builds are broken and need repairing.
  • Improved maintainability and scalability of CI configuration.
  • Reduced time for regression checks and overall CI pipeline execution.
  • Enhanced contributor experience by providing more informative linting feedback.

How was it changed?

  • Updates were made to GitHub Actions job configuration files.

What testing was done for the changes?

  • Changes were validated on a forked repository with passing CI.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@sirknightj sirknightj added the ci label Nov 1, 2024
@sirknightj
Copy link
Contributor Author

Note: the linter job will fail for this PR and that's expected. Once #262 is merged, this is expected to pass.

Copy link

@unicornss unicornss left a comment

Choose a reason for hiding this comment

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

nice work on improving the CI.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@stefankiesz
Copy link
Contributor

Looks like an unintended consequence of the DRY-principle changes is that it is no longer clear what particular test configuration if failing/passing from the title of the test. Anything that we can do about that?

image

image

@sirknightj
Copy link
Contributor Author

sirknightj commented Nov 4, 2024

I'll adjust the names of each case to be more concise.

It will now be in the format: OS, compiler, build type

@sirknightj sirknightj merged commit bac2318 into develop Nov 4, 2024
25 checks passed
@sirknightj sirknightj deleted the linter-enhancements branch November 5, 2024 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants