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

[repo] CI improvements #5023

Merged
merged 38 commits into from
Jan 8, 2024
Merged

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Nov 3, 2023

Changes

  • We recently overhauled the CI on the contrib repo to support more granular selection of jobs/tasks so that we can run subsets of the pipeline based on what is being changed. This PR brings that set up over to the main repo. We no longer need the "-md" versions of workflows and we can now be more selective about what we run.

    The main workflow "ci.yml" now decides what to run based on the files being changed. High-level overview:

    • We run the sanity check job for all changes.
    • If a ".md" file is changed we run the markdown lint job.
    • If some code artifact is changed (".cs", ".csproj", ".editorconfig") we run the dotnet format lint job.
    • If code is changed or something in the build folder is changed we run a build+test of the solution in stable & experimental modes.
    • If code is changed for an instrumentation package we run a build+test of the InstrumentationLibraries.proj in stable & experimental modes against the stable NuGet versions of dependencies.
    • If OTLP exporter code is changed (or one of its dependencies) we run the OTLP integration tests.
    • If api, sdk, or instrumentation is changed we run the W3C integration tests. This is too broad IMO and needs further work.
    • If any packaged code is changed we run the package validation workflow.
    • If any packaged code is changed we run the docfx workflow.
    • If any packaged code is changed we run the AOT verification workflow.

    All build+test runs have been refactored into a reusable workflow Component.BuildTest.yml. The main goal with that is to reduce the amount duplicated code and spots that need to be updated when we add a new TFM. All the steps also now have unique names so the checks UI is easier to read. After this PR the only required action will be "build-test" which is the step in the main workflow which joins everything together once complete.

@CodeBlanch CodeBlanch requested a review from a team November 3, 2023 16:24
Copy link

codecov bot commented Nov 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6250307) 83.38% compared to head (c8a78fc) 83.18%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5023      +/-   ##
==========================================
- Coverage   83.38%   83.18%   -0.21%     
==========================================
  Files         297      271      -26     
  Lines       12531    11968     -563     
==========================================
- Hits        10449     9955     -494     
+ Misses       2082     2013      -69     
Flag Coverage Δ
unittests ?
unittests-Instrumentation-Experimental 25.05% <ø> (?)
unittests-Instrumentation-Stable 25.05% <ø> (?)
unittests-Solution-Experimental 83.10% <ø> (?)
unittests-Solution-Stable 83.16% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 32 files with indirect coverage changes

Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Nov 11, 2023
@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Nov 16, 2023
@utpilla
Copy link
Contributor

utpilla commented Nov 17, 2023

@CodeBlanch Could you also do this for the Coyote checks introduced in #4879?

Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Nov 28, 2023
@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Nov 29, 2023
@TimothyMothra
Copy link
Contributor

Love this!
If I could add one thing, I'd like to see a brief comment at the top of workflows that describes ci.yml as the entry point that invokes all others. I also like to include one link at the top of workflows that best describes how they work. Seems this is the most appropriate: https://docs.github.com/en/actions/using-workflows/reusing-workflows#creating-a-reusable-workflow

@CodeBlanch
Copy link
Member Author

@TimothyMothra I pushed an update adding some comments into the child workflows.

packaged-code: ['src/**', '!**/*.md']
api-code: ['*/OpenTelemetry.Api*/**', '!**/*.md']
api-packages: ['src/OpenTelemetry.Api*/**', '!**/*.md']
instrumentation: ['*/OpenTelemetry.Instrumentation*/**', 'test/TestApp.AspNetCore/**', '!**/*.md']
Copy link
Member

Choose a reason for hiding this comment

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

given this repo only has well defined components this is okay. The contrib repo needs some changes to the wording in https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/CONTRIBUTING.md#contributing-a-new-project I believe, so that new components get CI covered. Not a blocker for this PR, just something to be addressed in contrib repo.

Copy link
Contributor

@utpilla utpilla Dec 16, 2023

Choose a reason for hiding this comment

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

Any new component added in the contrib repo would still get CI covered. It's just that if the new component does not a component specific proj file, any changes to the component would trigger CI for every project in the solution that does not have a dedicated proj file. That's because the filtering logic would determine that the CI needs to run for every project that does not have a dedicated proj file.

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should update the CONTRIBUTING.md to mention that new contributios could have a more efficient CI check by introducing a dedicated proj file for their component.

@utpilla
Copy link
Contributor

utpilla commented Dec 15, 2023

@CodeBlanch Need this change here as well? open-telemetry/opentelemetry-dotnet-contrib#1487

@CodeBlanch
Copy link
Member Author

@utpilla

Need this change here as well? open-telemetry/opentelemetry-dotnet-contrib#1487

Good question! We don't need the powershell part because we aren't using that in this repo. The other bits from that PR...

- "build"
- "docs"
- "examples"
- "src/Shared"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should shared files under src be excluded?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. This was copied from contrib but probably shouldn't be here. I'm thinking I'm going to leave it alone to see how codecov handles the new ci and come back to this later to smooth out any rough edges. I haven't figured out a way to make codecov really useful yet on contrib either 🤣

unittests-Instrumentation-Stable:
carryforward: true
paths:
- src
Copy link
Contributor

Choose a reason for hiding this comment

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

Update path for instrumentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure on this either. InstrumentationLibraries.proj actually builds and tests everything so this should be fine but 🤷 I'm considering code coverage as a WIP!

Copy link
Contributor

@utpilla utpilla left a comment

Choose a reason for hiding this comment

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

This is a great improvement! Left some comments about codecov.yml which if need to be addressed could be addressed in another PR.

Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added Stale Issues and pull requests which have been flagged for closing due to inactivity and removed Stale Issues and pull requests which have been flagged for closing due to inactivity labels Dec 30, 2023
Copy link
Contributor

github-actions bot commented Jan 7, 2024

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jan 7, 2024
@CodeBlanch CodeBlanch merged commit 88493d2 into open-telemetry:main Jan 8, 2024
48 checks passed
@CodeBlanch CodeBlanch deleted the ci-improvements branch January 8, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants