github-ci: don't run ee tests for outside pull requests by default

[oleg-jukovec]

Pull requests from outside collaborators don't have access to the repository secrets, see #156 as example.

I am not familiar with github actions. So I just copy a solution from crud and tt. I'm ready to do something different if you have an example or a better idea.

[Totktonada]

(This PR looks good per se. Let me know, whether you want to work on the idea described below.)

I have one idea here.

We can add a label like full-ci to trigger testing on Tarantool Enterprise. This way we can easily run remaining testing jobs in an external pull request without re-pushing the commits to a temporary branch in the main repository. A good point here is that this way we can ensure that the patchset has no malicious changes before approve the testing, where secrets are needed. Only persons with write access can set a label on a pull request.

NB: If we'll do, we should take care to the following case. A black hat opens a good pull request, we set full-ci label on it. Next, the black hat pushes malicious changes. The testing should not re-run automatically. It would be also good if the full-ci label will be unset at updating of an external pull request.

https://github.com/tarantool/tarantool actions work in the similar way, we can consider them as example.

[oleg-jukovec]

Thank you for the idea!

We can add a label like full-ci to trigger testing on Tarantool Enterprise. This way we can easily run remaining testing jobs in an external pull request without re-pushing the commits to a temporary branch in the main repository. A good point here is that this way we can ensure that the patchset has no malicious changes before approve the testing, where secrets are needed. Only persons with write access can set a label on a pull request.

There is a simple example with a label safe for test.

NB: If we'll do, we should take care to the following case. A black hat opens a good pull request, we set full-ci label on it. Next, the black hat pushes malicious changes. The testing should not re-run automatically. It would be also good if the full-ci label will be unset at updating of an external pull request.

I agree.

But I have a problem. I can assign a label, but I can't find a way and can't undestand how to grant access to secrets from the repository to the pull request. I don't see any button or something else to do it (maybe I just don't have enough permissions).

[Totktonada]

But I have a problem. I can assign a label, but I can't find a way and can't undestand how to grant access to secrets from the repository to the pull request. I don't see any button or something else to do it (maybe I just don't have enough permissions).

AFAIU, if we trigger jobs at labeling a pull request, it is the different event, where we have access to secrets.

[oleg-jukovec]

AFAIU, if we trigger jobs at labeling a pull request, it is the different event, where we have access to secrets.

Could you please add the full-ci label? I will merge the pull request to the master and we can to resume.

Unfortunately, we can't test the behavior here:

This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does.

But I think it should work.

[Totktonada]

Could you please add the full-ci label?

Done.

[Totktonada]

Will not run-tests-ce be triggered on the labeled event?

  run-tests-ce:
    <...>
    if: github.event_name == 'push' ||
github.event.pull_request.head.repo.full_name != github.repository

This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does.

But I think it should work.

Hm. It'll test master, right? We should checkout necessary branch in run-tests-ee expilictly, when it is run on the labeled event, as I understand.

[oleg-jukovec]

Done.

Thank you!

Will not run-tests-ce be triggered on the labeled event?

Yes, it will. Fixed. I have listed events in the condition. I hope this will help avoid mistakes in the future.

Hm. It'll test master, right? We should checkout necessary branch in run-tests-ee expilictly, when it is run on the labeled event, as I understand.

Unfortunately yes, I have tested the case and added additional options to a checkout.

[Totktonada]

Hm. It'll test master, right? We should checkout necessary branch in run-tests-ee expilictly, when it is run on the labeled event, as I understand.

Unfortunately yes, I have tested the case and added additional options to a checkout.

Should not we add the checkout action parameters only for the pull_request_target event? I guess that something may go wrong for push and workflow_dispatch events with those parameters. But maybe you know more, that's just guess. If some trick is used here and everything is fine, I vote for a short explanation comment.

---

I would leave some comments to the AND/OR conditions: they become larger and now it is not easy to spot the idea at glance.

[Totktonada]

I don't see any problems now. I suggest to wait someone else to glance and proceed.

[oleg-jukovec]

Should not we add the checkout action parameters only for the pull_request_target event? I guess that something may go wrong for push and workflow_dispatch events with those parameters. But maybe you know more, that's just guess. If some trick is used here and everything is fine, I vote for a short explanation comment.

It works fine for this cases (see Clone the connector steps for failed run-tests-ee):
https://github.com/oleg-jukovec/go-tarantool/pull/3
https://github.com/oleg-jukovec/go-tarantool/actions/runs/2440709670

Thank you for the review. I started to understand something (but not too much) in github actions logic.

[vr009]

LGTM.