Improve PR review process

[ovivoda]

Hi all,

I saw that there are quite a few Actions workflows defined for the repository.

I think we should start categorising the workflows and decide which ones are mandatory. Maybe all are....

The problem is: too many PR's do not have green light from CI/CD. For this I think we can:

a) Keep the PR's in draft until are validated by the CI/CD. Once we have green light there we should ask for a review.
or
b) Automate actions workflows in our own fork, then have the CI/CD running there, and once we have a green light, publish a PR and ask for review.
or
c) you name it please....

Of course we can differentiate/automate this by platform, folders where the changes are taking place, etc...

Any feedback is appreciated. I'm sure there are other options as well.

[ntadej]

I think there is no drawback running many workflows. We optimised them quite a lot already and I guess more optimisations can be done.

It is important though to try to reach stability of some of the workflows (e.g. node tests can spuriously fail).

[ntadej]

Draft status should indicate readiness. I agree that for some PRs this should happen (e.g. #702 was never draft if I remember correctly).

For small fixes and especially external contributors we should make the process as simple as possible.

Note also that draft status does not prevent pipelines by default and often one would also want to run pipelines.

[ovivoda]

Can you please define readiness?
Can you please define small fixes?
We can run the pipeline on a draft PR can't we?

[ntadej]

readiness = developer thinks the code is ready (Draft is the status the developer should use to indicate when the review should start)
small fixes = hard to define, I guess PRs where only a few lines of code are changed and/or all changes are of the same nature

Pipelines already run on draft PRs, but I guess we could disable that (I'm against it).

[ovivoda]

@ntadej Thanks!

Regarding readiness: based on Github docs I think a draft PR is a reflecting that is still not ready for review. More on this here: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request

Regarding small fixes: In my opinion things are like this: As far as we allow exceptions then there is no rule.

Regarding pipelines in drafts: I totally agree that we should run them there. It is a good way for the developer that started the PR to see if pipelines are running with success.

@ntadej I appreciate your feedback on this!

[wipfli]

If CI is failing, a pull request cannot be merged, because otherwise our main branch has broken CI which is not what we want. So the maintainers task is to only approve and merge pull requests which have passing CI.

If CI is broken due to reasons unrelated to the pull request, the maintainers should fix the broken CI even if it is not an android or iOS pipeline which fails. Maybe this will require turning off flaky tests on non-core platform binding and making a ticket about it. I don't know. But our goal should be to always have a green tick when you come to this repository.

Running CI on draft pull requests is fine. If somebody works on a pull request and would like to indicate that the pull request is not ready for review, they make it a draft. Maintainers can safely ignore draft pull requests. And once it is ready for review, the author can remove the draft status and write a message like "ready for review"

[thehoneymad]

I am aligned with @ovivoda here. But I want to clarify my viewpoint of alignment.

I think we have Github Action files with duplicated code. For example android-ci-pull and android-ci. They are near alike. One is triggered on publishing PR and it does not publish the testing app. The other happens after commit, does everything the PR one does, and publishes the test app.

@ovivoda I wanted to write a CI design for Android. But I am too bogged down with work. So I would love if you write it. In my mind, the design should:

1. Address reusable parts of the CI workflow of Android. Such as building the core, building the android SDK, publishing test app.
2. Use Github Reusable Workflow. A great documentation is here.
3. Work with @louwers and @wipfli to propose a technical design for reproducible builds locally.
4. How to build the plugin libraries in separate git repositories when Core or Android SDK is built to guarantee backward compatibility.
5. And finally, publish your design as an addendum of the rust/markdown book. You can also propose how, where, what CI should be run to ensure build correctness.

[louwers]

I am not even sure if the additional steps of android-ci are even relevant still.

Sounds good, we'll get back to you. @ovivoda and I have a meeting again next Tuesday.

I created an issue for the first reusable workflow: #735.

reproducible builds locally

Why is this important / what do you mean with this exactly?

[ovivoda]

@thehoneymad thanks for the feedback! Indeed I will start digging some more into GitHub actions and see how can we improve the CI workflow.