Skip to content

Conversation

@arielvalentin
Copy link
Contributor

@arielvalentin arielvalentin commented Oct 28, 2025

This should skip running tests for cases where it is not needed

Fixes: #1753

Skipping changes to instrumentation-with-services:

image

This should skip running tests for cases where it is not needed

Fixes: open-telemetry#1753
- Created ci-instrumentation-all.yml to handle the 'all' instrumentation
- This workflow triggers on any instrumentation/** or helpers/** changes
- Removed 'all' from ci-instrumentation.yml to avoid duplication
- Ensures 'all' instrumentation tests run for any instrumentation change
  as required for merging while maintaining CI optimization benefits
@arielvalentin arielvalentin marked this pull request as ready for review October 28, 2025 04:32
branches:
- main
paths:
- 'instrumentation/action_mailer/**'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will still trigger all instrumentation test to run even if I only modified the file inside e.g. http/.

Just an idea: the matrix can be modified

  detect-changes:
    runs-on: ubuntu-latest
    outputs:
      matrix: ${{ steps.set-matrix.outputs.matrix }}
    steps:
      ...

      - name: Get changed instrumentation files
        id: changed-files
        run: |
          if [[ "${{ github.event_name }}" == "schedule" ]] || [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then
            echo "run_all=true" >> $GITHUB_OUTPUT
          else
            echo "run_all=false" >> $GITHUB_OUTPUT
            echo "changed_gems=$(git diff --name-only ...)" >> $GITHUB_OUTPUT
          fi
      
      - name: Set matrix
        id: set-matrix
        run: |
          # get CHANGED_GEMS from last step
          echo "matrix={\"gem\":$CHANGED_GEMS,\"os\":[\"ubuntu-latest\"]}" >> $GITHUB_OUTPUT

  instrumentation:
    needs: detect-changes
    if: ${{ needs.detect-changes.outputs.matrix != '' && fromJson(needs.detect-changes.outputs.matrix).gem[0] != null }}
    strategy:
      fail-fast: false
      matrix: ${{ fromJson(needs.detect-changes.outputs.matrix) }}

    name: ${{ matrix.gem }} / ${{ matrix.os }}
    runs-on: ${{ matrix.os }}
    steps:
      - uses: actions/checkout@v5
      - name: "Test Ruby 3.4"
        ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see what you mean.

If an instrumentation changes, there are too many interdependencies at the moment between helpers, frameworks and HTTP etc.. so I do want them to run together.

For example, if I change instrumentation/active_support I also want to tests for anything that depends on that gem e.g. action_view.

It is a bit difficult to encode that as a set of dependencies to make sure they all run together.

Now that I think about it, changes to instrumentation/base and helpers should also trigger Instrumentation with services since those are critical dependencies for those libraries as well.

branches:
- main
paths:
- 'helpers/**'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as last comment, if I only modified resources/, I probably only want resources/ to run test.

With paths-filter, we can use the output from detect-changes to decide on what jobs to run

jobs:
  detect-changes:
    runs-on: ubuntu-latest
    outputs:
      helpers: ${{ steps.filter.outputs.helpers }}
      propagators: ${{ steps.filter.outputs.propagators }}
      ...
    steps:
      - uses: actions/checkout@v5
      - uses: dorny/paths-filter@v3
        id: filter
        with:
          filters: |
            helpers:
              - 'helpers/**'
            propagators:
              - 'propagator/**'
            ...

  helpers:
    needs: detect-changes
    if: ${{ needs.detect-changes.outputs.helpers == 'true' || github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' }}

  propagators:
    ...

@arielvalentin arielvalentin marked this pull request as draft October 28, 2025 14:54
@arielvalentin arielvalentin marked this pull request as ready for review October 28, 2025 15:00
@arielvalentin arielvalentin self-assigned this Oct 28, 2025
@arielvalentin arielvalentin marked this pull request as draft October 28, 2025 17:21
@arielvalentin
Copy link
Contributor Author

Reviewed in the SIG and I missed a few paths triggers so I am going to see if I could use anchors to avoid structural duplication

@arielvalentin arielvalentin marked this pull request as ready for review October 29, 2025 16:38
@arielvalentin
Copy link
Contributor Author

@open-telemetry/ruby-contrib-approvers per our chat during the SIG, I have removed duplication in the actions workflows by using YAML anchors.

Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @arielvalentin! This seems like a huge improvement. I have a few questions before I approve.

@arielvalentin arielvalentin merged commit 1186594 into open-telemetry:main Oct 30, 2025
62 checks passed
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.

Proposal to Refine CI Triggers

3 participants