Actions: Fix handling of paths-ignore in autobuild scripts, add integration tests for configured path filters #19278
Conversation
Create a common file structure to be shared among multiple tests for path filters, and rename accordingly. Update test expectations with additional files. Use pytest markers to indicate the expected outputs. Add source archive checking in addition to checking query output. This allows us to test which YAML files were extracted separately from whether they are semantically meaningful to the Actions analysis.
Use the common structure from the existing test for default filters. Check both query output finding workflows and actions, and source archive output showing all extracted YAML files. The test for only `paths-ignore` fails in this commit, demonstrating a bug: we start with all YAML files rather than starting with the default includes. The tests for `paths` reflect current behaviour which is consistent with other languages: `paths` overrides the default inclusions, and only files under `paths` are included. This may not be the best user experience for Actions, since we want to scan all workflow and action files even in the presence of `paths`, but that is not currently addressed.
Always concatenate the default filters with the user-provided filters. This ensures that when `paths-ignore` is provided, we begin with the default path inclusions, not all YAML files. This makes the `paths-ignore-only` integration test variant under `filters` pass. The handling of `paths` is unchanged: if provided, this overrides the default filters.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the handling of paths-ignore in autobuild scripts by concatenating the default filters with user-provided filters, and introduces integration tests to verify the behavior of configured path filters. Key changes include the addition of three new configuration files for paths, paths-ignore, and a combination of both, as well as the removal of a now obsolete default filter test.
Reviewed Changes
Copilot reviewed 16 out of 27 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| actions/ql/integration-tests/filters/codeql-config.paths-only.yml | Adds a configuration file testing inclusion via the "paths" filter. |
| actions/ql/integration-tests/filters/codeql-config.paths-ignore-only.yml | Adds a configuration file testing exclusion via the "paths-ignore" filter. |
| actions/ql/integration-tests/filters/codeql-config.paths-and-paths-ignore.yml | Adds a configuration file testing both filters in combination. |
| actions/ql/integration-tests/filters-default/test.py | Removes an outdated test that no longer aligns with the new filter concatenation logic. |
Files not reviewed (11)
- actions/extractor/tools/autobuild-impl.ps1: Language not supported
- actions/extractor/tools/autobuild.sh: Language not supported
- actions/ql/integration-tests/filters/actions.default-filters.expected: Language not supported
- actions/ql/integration-tests/filters/actions.paths-and-paths-ignore.expected: Language not supported
- actions/ql/integration-tests/filters/actions.paths-ignore-only.expected: Language not supported
- actions/ql/integration-tests/filters/actions.paths-only.expected: Language not supported
- actions/ql/integration-tests/filters/actions.ql: Language not supported
- actions/ql/integration-tests/filters/source_archive.default-filters.expected: Language not supported
- actions/ql/integration-tests/filters/source_archive.paths-and-paths-ignore.expected: Language not supported
- actions/ql/integration-tests/filters/source_archive.paths-ignore-only.expected: Language not supported
- actions/ql/integration-tests/filters/source_archive.paths-only.expected: Language not supported
|
|
||
| if ($null -ne $env:LGTM_INDEX_FILTERS) { |
There was a problem hiding this comment.
If $env:LGTM_INDEX_FILTERS is an empty string, does this still work?
There was a problem hiding this comment.
Also, LGTM_INDEX_FILTERS includes both the include and exclude filters?
There was a problem hiding this comment.
Also, LGTM_INDEX_FILTERS includes both the include and exclude filters?
Yes - LGTM_INDEX_FILTERS is a string containing include:<pattern> and exclude:<pattern> substrings separated by newlines.
There was a problem hiding this comment.
If $env:LGTM_INDEX_FILTERS is an empty string, does this still work?
$null -ne $env:LGTM_INDEX_FILTERS is True in this case, and then we concatenate the default filters with an extra newline at the end, so it still works.
This PR is an attempt at a minimal solution for the backlinked internal issue, where we extract too many YAML files when the user provides
paths-ignore, due to ignoring the default inclusions. It includes integration tests to understand the problem and current behaviour.In the interest of simplicity, I did not attempt to change the handling of
paths, which currently override the default inclusions. This may not be the best user experience for Actions, since we want to scan all workflow and action files even in the presence ofpaths, but that is not currently addressed.Key changes:
pathsreflect current behaviour which is consistent with other languages:pathsoverrides the default inclusions, and only files underpathsare included.paths-ignorefails in the commit that introduces the test, demonstrating a bug: we start with all YAML files rather than starting with the default includes.paths-ignoreis provided, we begin with the default path inclusions, not all YAML files. This makes thepaths-ignore-onlyintegration test variant underfilterspass.