Allow to use ansible-test's change detection for Pull Requests

[felixfontein]

Test PR: felixfontein/ansible-playground#4

This is necessary to make ansible-collections/community.crypto#520 bearable (running all integration tests needs 20-30 minutes), and to make a similar thing work for community.general and other larger collections where you usually don't want to run everything in CI.

(Not sending code coverage in this case is important since not all tests are run.)

The tricky part here is using ansible-test's change detection since it expects the current commit to be HEAD of a branch, and it expects the branch it is branched from to be around. GitHub's checkout action provides neither, but in the case of pull requests we have the merge commit checked out. By passing fetch-depth: 0 to the checkout action, we can get all history, and we also get the branches from the repository where this PR runs in. This allows us to checkout origin/<target_branch_name> (usually main) as <target_branch_name>, and we can create a new branch __action_branch_name for the current commit (the merge commit). We then can call ansible-test with --changed --base-branch <target_branch_name> and let ansible-test handle the change detection.

https://github.com/felixfontein/ansible-playground/actions/runs/3352362049/jobs/5554455066 shows how this works. In the PR's branch one file has been modified (plugins/connection/dummy.py), while in the current main branch of the collection also another file has been modified (the GHA workflow). So ansible-test should only detect the change in the plugin and not in the workflow (if you naively do a git diff --name-only origin/<target_branch_name> you get both files). The relevant lines are:

Detected branch __action_branch_name forked from main at commit 2ea3f69b761ca43575e749acc0c0fc79e43fb4e2
Detected changes in 1 file(s).
plugins/connection/dummy.py
Mapping 1 changed file(s) to tests.

(Note that the linked run was with a slightly older version of this branch, which included a git log statement. That's gone now.)

[felixfontein]

FYI, this branch is currently used in community.crypto's main branch.

[felixfontein]

@webknjaz can you take a look at this one?

[webknjaz]

Oh, I somehow missed this one. Will take a look, thanks for the ping.

[webknjaz]

Any reason not to have this on by default?

[felixfontein]

It's a breaking change, and different what most collections do right now. Basically only the collections using AZP use change detection, all others simply run all tests.

Also switching this on effectively disables code coverage reporting for pull requests.

[webknjaz]

Also switching this on effectively disables code coverage reporting for pull requests.

Could you explore if codecov's carryforward feature could fix this?

[webknjaz]

@felixfontein the new input probably needs a README entry and some test job?

[felixfontein]

Testing isn't so easy, since the test cases run with collection-src-directory set. I can adjust the tests to still run, but I think it would be better to make the action fail if the new option is used together with collection-src-directory.

[webknjaz]

(Not sending code coverage in this case is important since not all tests are run.)

It sounds like Codecov's Carryforward flags could handle this:

• https://docs.codecov.com/docs/flags#carryforward-flags
• https://docs.codecov.com/docs/carryforward-flags

[webknjaz]

Testing isn't so easy, since the test cases run with collection-src-directory set. I can adjust the tests to still run, but I think it would be better to make the action fail if the new option is used together with collection-src-directory.

You could add another matrix job with it unset, or set to '' which should be the same essentially.

[felixfontein]

(Not sending code coverage in this case is important since not all tests are run.)

It sounds like Codecov's Carryforward flags could handle this:

* https://docs.codecov.com/docs/flags#carryforward-flags

* https://docs.codecov.com/docs/carryforward-flags

I cannot claim I fully understood carryforward flags, but the way I understand them they won't help here at all, since ansible-test --changed determines on a per-file basis which tests to run.

Also besides that, I really don't want any code coverage to be uploaded for PRs in the collections where I plan to use this.

[felixfontein]

Testing isn't so easy, since the test cases run with collection-src-directory set. I can adjust the tests to still run, but I think it would be better to make the action fail if the new option is used together with collection-src-directory.

You could add another matrix job with it unset, or set to '' which should be the same essentially.

Setting it to '' (or unsetting it) will make the tests fail. Basically that option is necessary to be able to run tests in this repository at all, since the test collection used in this repository is not in the repository's root. Ah, that's not right. I confused collection-src-directory with collection-root apparently... I've tried it out in 0331670.

[felixfontein]

The tests now pass, and the life test of this branch in felixfontein/ansible-playground#4 is still working.

[felixfontein]

I created a new PR #48 for two of the latter commits (68c8263, 1c9d960), since they can also be nice (no unnecessary copying) unrelated to this PR.

[webknjaz]

@felixfontein I've accidentally merged the wrong branch so I had to force-push to reset that quickly. You'll need to reopen this as a new PR.

[webknjaz]

I cannot claim I fully understood carryforward flags, but the way I understand them they won't help here at all, since ansible-test --changed determines on a per-file basis which tests to run.

So I've read the docs again myself, and it appears that it'd help, if one were to list those flags in a config and set paths: to .*.

Also besides that, I really don't want any code coverage to be uploaded for PRs in the collections where I plan to use this.

I wonder why. Is that because you think that their bot comments/statuses would be annoying? I think that could be disabled, and it'd be made silent.

Also, I can imagine that some people would still want this, so maybe it's reasonable to have a flag/input for this? Or autodetect the use of carryforward in the codecov config? If there's any carryforward: true there, then default to uploading coverage in PRs, otherwise, default to skipping that step.

[felixfontein]

I've re-opened this in #49.