Test all supported configurations after merge/validate

[qstokkink]

IPv8 is supposed to run for all combinations of OS Linux/Windows/Mac and Python versions 3.7/3.8/3.9/3.10. We only test a select subset of these combinations for PRs (namely Linux+Python3.7, Windows+Python 3.8 and Mac+Python3.8). Because some combinations are not tested, this could lead to new code introducing hidden incompatibilities.

To assure that IPv8 runs on all the operating systems and Python versions that we claim to support, we should run the unit tests on all of the supported OS+Python combinations for every merged PR (either just before or just after merging).

[drew2a]

One of the easiest ways to do it is to use Travis Build Matrix

[qstokkink]

We should probably also run the entire test suite on each build environment twice: for IPv4 and IPv6. For reference: the tests use IPv4 (default) if ipv8.test.mocking.endpoint.ADDRESS_TYPE = "UDPv4Address" and IPv6 if ipv8.test.mocking.endpoint.ADDRESS_TYPE = "UDPv6Address". At the time of writing, the latter is still expected to break until #615 is resolved.

[qstokkink]

We should probably also make sure to test all supported test runners: unittest (which is the most important), nose and pytest. I'm not sure if other runners are (still) supported, perhaps trial?

[qstokkink]

Based on the amount of time taken by the Tribler PR tests (using GitHub Actions), unit testing would take in the order of minutes. A full run of all supported platforms would therefore make our tests an order of magnitude slower. This can be very annoying when you want quick feedback for easy errors (for example from pylint).

In order to not break our current development interaction speed, we should probably not execute all the new test runners for every change of a PR. I think there are two main options: (1) run the tests on all platforms when a PR is being validated (i.e., after a "validate" comment, or (2) after a PR has been merged.

[drew2a]

@qstokkink you might also want to consider the option (3). To divide checks into two groups and run the first group when PR is in the "draft" state, and the second when PR is in the "ready to review" state.

See the example of Tribler's PR checks:

  coverage:
    uses: ./.github/workflows/coverage.yml
    with:
      python-version: 3.8

  pytest:
    uses: ./.github/workflows/pytest.yml
    with:
      python-version: 3.8

  guitest:
    uses: ./.github/workflows/guitest.yml
    with:
      python-version: 3.8

  documentation:
    if: github.event.pull_request.draft == false
    uses: ./.github/workflows/documentation.yml
    with:
      python-version: 3.8

  ubuntu:
    if: github.event.pull_request.draft == false
    uses: ./.github/workflows/build_ubuntu.yml
    with:
      upload: false
      os: ubuntu-20.04
      python-version: 3.8

https://github.com/Tribler/tribler/blob/main/.github/workflows/!PR.yml

[qstokkink]

@drew2a thanks for the suggestion, I gave option 3 some serious thought over the last few days. My conclusion is that depending on draft/ready (which can be controlled by the user submitting the PR) does not fit our existing access levels, as users can then initiate a "heavy PR test" run without approval by team members that have (at least) triage permissions.

For reference, our current permission model looks like this (Click Me!)

Impact	Permission	Applies to	Access	Granted by
0	-	User	Create comments, issues, PRs	-
1	"ok to test"	Single PR	Run light PR tests	triage
1	"add to whitelist"	User	User is "ok to test" on all PRs	triage
2	"validate"	Single PR	Run heavy PR tests	triage
3	Approve PR	Single PR	Allow write to main branch	triage*
4	Merge PR	Single PR	Write to main branch	admin

*Currently broken and only available to admin

Impact	Description
0	No consequences for Tribler's infrastructure
1	Light load on Tribler's infrastructure
2	Heavy load on Tribler's infrastructure
3	Possibility for permanent damage to codebase
4	Permanent damage to codebase

If I understood option 3 correctly, it would add the following permission:

Impact	Permission	Applies to	Access	Granted by
2	-	User	Run heavy PR tests	-

The risk I see is that random users can put (our) infrastructure under heavy load without any intervention by triage. Thereby, if abused, a random user could get us banned from GitHub Actions or take down our own testing infrastructure. Currently, only triage members can initiate heavy load and I think it would be a good idea to keep it that way.

[drew2a]

Yes, you are right.

Why I suggested this option — because it is more natural for GitHub users to operate with terms and workflows they are familiar with. For me (as a GitHub user), these keywords ("add to whitelist", "ok to test", "validate") are quite confusing. I don't understand the workflow behind them.

But I could understand a workflow with "draft"->"ready for review" PR states naturally, without explanations or/and documentation. Also, with a bit more effort, I can understand workflows written in yml files.

That's why (in my opinion) this option at least should be mentioned here.

I will left two links (for the record) about how to restrict access to the PR checks and prevent heavy load on infrastructure:

• https://dev.to/petrsvihlik/using-environment-protection-rules-to-secure-secrets-when-building-external-forks-with-pullrequesttarget-hci
• https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

[qstokkink]

Thanks for the links. I have never used GitHub's Environments and it was interesting to read about them. However, this discussion has gone somewhat off-topic now. This issue is about hooking in extra "heavy" validation and not so much about modernizing the IPv8 PR workflow to integrate into GitHub. I'll offer some comments but further discussion on workflow modernization should move to a separate issue (please open a new issue if you want to discuss this further):

Comments on workflow modernization

Let me start with the disclaimer that most of the IPv8 workflow predates GitHub Actions by several years. It's old and mainly stems from the available tools in Jenkins. That's why it's more like Interactive Fiction than an integration dashboard 😃

I like the idea of integrating more with GitHub and it seems GitHub's Environments can enable this. However, what I gather from your linked material is that the workflow itself would not change with better integration into GitHub. Instead of commenting "ok to test" a triage member would have to click a button to allow testing. Instead of commenting "validate" a triage member would have to click a button to allow validation. These steps are the same. The difference I see lies in the interface triage members use to advance the workflow. Nevertheless, it would indeed be more convenient to press a button than to comment.

Even pressing a button gets obnoxious after a while. This is why "add to whitelist" exists. However, I assume there is some GitHub Action magic that allows for auto-workflow-approval for trusted users.

Lastly, the current workflow is indeed old but it still works. I do see merits in modernizing but "if it ain't broke, ~~don't fix it~~ ask management for permission" before we sink many hours into this.