CI: Add changed-files GitHub Action#111046
Conversation
|
Let's just add a dummy change temporarily to ensure this works correctly |
|
Dummy changes with a new file, renamed file, and modified file. The modified file was one of the few files in the entire repo with whitespace/"unsafe" characters in the name, so it doubles as a test to ensure those are processed correctly |
|
The results in a pull request context are entirely as expected: The local push changes are technically working as expected, but reflect a potential inherent limitation where only the most recent commit is compared against ( Having said that, there's an obscene number of inputs this action can accept, so it might be feasible to let it compare against the main branch of the repo instead. I'll do some testing today to verify |
|
Should compare against the base branch or things will break badly when 4.6 branches off etc. |
c3c5f25 to
204b7ac
Compare
14e8f3a to
419cc73
Compare
ec4aca8 to
4032624
Compare
a97a6b7 to
f33fcd2
Compare
|
Finally got back to this & to a point that I'm totally satisfied with the output. I've accepted pushes falling back to the previous commit as a limitation of git itself, and will drop the idea of attempting upstream hacks (for now…) While there's absolutely potential for more specialized output, I think this is a reasonable starting point. Future PRs can build off this action now that it exists, meaning conditionally enabling/disabling tests based on what's been changed (docs-only not running builds, android-only building just android, etc) |
bb0bfe2 to
412d4a9
Compare
e86e928 to
217c5e4
Compare
217c5e4 to
4e5ded7
Compare
|
Updated to have |
Ivorforce
left a comment
There was a problem hiding this comment.
Using an action for this sounds good with me if we've had issues with reliability of the shell script before.
I don't know what a "treeless clone" is but the runtime/speed of the action seems fine.
I'm impartial on style, so maybe needs a more opinionated review.
| inputs: | ||
| changed-files: | ||
| description: A list of changed files. | ||
| required: true | ||
| type: string |
There was a problem hiding this comment.
This change will make it impossible to call this workflow without providing it changed files to check. Maybe we should make it not required and act accordingly (skip the clang-tidy check)?
There was a problem hiding this comment.
It should be possible to call via workflow_dispatch, with the input variable falling back to an empty string. But as an added precaution, I'll see if I can skip that step on dispatch entirely
There was a problem hiding this comment.
Update: I verified on my local repository that this is indeed how it works. Given the workflow_dispatch exception might be tricker than expected, I'll just leave this as-is
Replaces our existing shell solution for determining changed files with a dedicated action: changed-files. While this process takes slightly longer than the shell script, it appears to have a much higher consistency with actually gathering the files. Most notably: this enables static checks on forks!