[chore] Add scoped GH action to test affected components on Windows #37106
Conversation
pjanotti
added
Skip Changelog
|
|
||
| - name: Get changed go files | ||
| id: changed-files | ||
| uses: tj-actions/changed-files@v45 |
There was a problem hiding this comment.
This action is already used to check links in markdown files.
There was a problem hiding this comment.
We have several actions that check for diffs and in the past have found that comparing directly against main causes a lot of pain because unrelated changes to main are detected in the diff. e.g. Your PR changes module X, but while you were working on it modules A, B, and C were updated by other PRs. Now your PR shows a diff of A, B, C, and X. This repo is one of the busiest in all of open source, so it tends to surface this type of problem more than most.
The solution we're relied on is to diff against the most recent common ancestor commit (via git merge-base). I took just a quick look at tj-actions/changed-files and I think it's not using this strategy, though it does perform a boolean "can diff" check based on it.
In any case, I think it's fine to start here but I wanted to note that we should keep an eye out for this kind of issue and may need to change how we determine the diff.
There was a problem hiding this comment.
Thanks for the info, I will keep an eye on this potential issue.
| key: go-cache-${{ runner.os }}-${{ hashFiles('**/go.sum') }} | ||
|
|
||
| - name: Run changed tests | ||
| if: needs.changedfiles.outputs.go_tests |
There was a problem hiding this comment.
This is added to cover the case of only changes affecting test files. If the step running tests on dependent components hit the same test it will used the cached results.
| env: | ||
| CHANGED_GOLANG_SOURCES: ${{ needs.changedfiles.outputs.go_sources }} | ||
| run: | | ||
| make for-affected-components CMD="make test" |
There was a problem hiding this comment.
Initially I wanted to run CMD="make" to run the default target of the component, however, some targets break on Windows due to the size of the command line - to be investigated separately.
| echo "No go source changes detected in shippable code."; \ | ||
| else \ | ||
| cd $(SRC_ROOT); \ | ||
| DEPENDENT_PKGS=$$(echo $(CHANGED_GOLANG_SOURCES) | xargs sed -n 's|^package .* // import "\(.*\)"$$|\1|p' | uniq); \ |
There was a problem hiding this comment.
This stops at the components directly dependent on the components being changed. In the future we can consider running through all levels of dependent code, eventually hitting the collector itself. However, I wanted to start small and see what I may have missed in this change.
|
|
||
| - name: Get changed go files | ||
| id: changed-files | ||
| uses: tj-actions/changed-files@v45 |
There was a problem hiding this comment.
We have several actions that check for diffs and in the past have found that comparing directly against main causes a lot of pain because unrelated changes to main are detected in the diff. e.g. Your PR changes module X, but while you were working on it modules A, B, and C were updated by other PRs. Now your PR shows a diff of A, B, C, and X. This repo is one of the busiest in all of open source, so it tends to surface this type of problem more than most.
The solution we're relied on is to diff against the most recent common ancestor commit (via git merge-base). I took just a quick look at tj-actions/changed-files and I think it's not using this strategy, though it does perform a boolean "can diff" check based on it.
In any case, I think it's fine to start here but I wanted to note that we should keep an eye out for this kind of issue and may need to change how we determine the diff.
Description
Many times changes that break Windows CI are merged since it is not apparent that they will break on Windows. Given the cost of running Windows CI for every PR the label
Run Windowsis only added in few cases. This change adds make targets that allow to run make commands only for tests and dependencies affected by files being changed.It uses a simple detection of go files that were changed and look to the import commands to find the components that depend directly on the package being changed.
Link to tracking issue
Relates to #36788
Testing
Validated on my fork, e.g.: https://github.com/pjanotti/opentelemetry-service-contrib/actions/runs/12659871185
Documentation
N/A