I’ll come back to the bigger issue in a separate reply >> - It wasn’t clear to me from this vote thread, that by design, we will allow >> some PRs to pass based on the CI status reported from the fork. > > This is a misunderstanding. It becomes clearer when you look deeper into how > GitHub actions work. > > It's the status set by "OUR" workflow run from main in our repo. It looks at > the status of tests in the fork. And it will not do it if the PR changed any > of our workflows. And it is **better** than today. Currently **any** PR and > contributor can make a change to current workflows and tests and they will be > "green" when run in our CI. By implementing this change we are **preventing** > this because we will not let status go green without specific approval by > maintiner,
Sure, the status is reported by _our_ workflow, but it is relaying/trusting the status reported by the CI running on the fork. This is what I mean by “based on the CI status reported from the fork”. i.e. I though we were going to have people run CI on their forks, and then always, before every single PR was merged, require the CI to run on the project CI too. That is what was surprising to me. >> - This tests something different to the CI in the apache/airflow repo on PR >> in one very important way. > > Nope. It's the same as today. See above ^^. The tests are **exactly** the > same - and by those changes we even make it harded for contributors to > "bypass" our CI. > >> When the CI is run on a PR in apache/airflow, this is running the CI >> against the merge commit of the PR to the target branch >> (`refs/pull/N/merge`), but the run ci in fork is just against the tip of the >> branch. > > No. It has never been like that. For about 4 years all our PRs have been > using the head of the PR. This is often why we tell people to rebase. And it > does not matter too much, because we often merge those PRs weeks after their > jobs were run, so they are outdated anyway today. Every day makes it drift > further away. There are ways we can change it - for example we might > **require** rebasing to latest main before merging (this is the same for > current and the new approach). And we can do it today - but we would never > do it. Because that would mean we would merge only a few lucky PRs. We can > definitely improve that - for example add extra check for "staleness" of the > PR and even see if the area of the PR has been modified since last PR - but > only **after** we merge AIP-120. I will add a feature for that. Oh really? Go to https://github.com/apache/airflow/actions/runs/42056562174/job/127348624454 and expand “Checkout target branch” and tell me what you see. Project CI, PR #74493, no fork label: https://github.com/apache/airflow/actions/runs/42056562174/job/127348624454 git checkout --progress --force refs/remotes/pull/74493/merge Fork CI, PR #76536, "use fork ci": https://github.com/ashb/airflow/actions/runs/42029244655/job/127251731725 git checkout --progress --force -B execution-api-xcom-params-in-datamodels \ refs/remotes/origin/execution-api-xcom-params-in-datamodels One tests the branch merged into main. The other tests the tip of the branch. The same difference applies to every test job, e.g. “Postgres tests: core". The workflow file is the same; what is checked out to build the image and run the tests is not. Given how much time you spend in our CI I am honestly gobsmacked that you didn’t know this basic fact about GH Actions. > >> This one is probably fixable by changing the ci-amd.yaml to notice when >> it’s on a fork and finding the matching PR somehow. >> >> - Frankly I find the “set the PR to draft” instantly on a CI failure abusive >> and hostile. > > Oh - this was a bug - and should be fixed already. If not, point me to it. It’s my test PR https://github.com/apache/airflow/pull/76536, go back through the edit history of the comment from github-actions > >> - Adding "use project ci" doesn't stop a PR merging on the fork CI result, >> and with auto-merge enabled that's wide enough to drive a >> bus through. > > No. Again it's **safer** than today. Today, a contributor can do "sneaky" > things in their PR—modify the workflow a bit and make it pass immediately. > Nothing can prevent it today. AIP-120 improves it. As a maintainer, you will > have to manually approve workflows for a PR that changes a workflow to pass > the initial gate and you will also get a comment saying "This PR changes CI > workflows - be careful". It literally _is not_. I did the _exact_ steps I listed below, and project CI never ran or blocked it from merging. Thank you so much for calling me a liar first and not spending the time to actually look at the issue I highlighted. It really makes me feel appreciated. >> >> So the sequence is: >> >> fork CI goes green >> -> add "use project ci" >> -> close/reopen >> -> approve with auto-merge on, or merge, before project CI finishes >> a-> it merges on the fork's "CI passed". >> >> I tested this: with project CI queued, the PR showed as mergeable, and >> the fork's status was the only "CI passed" on the commit. >> >> So to make sure a PR is validated by project CI, you have to remember to >> add the label, close and reopen, and then not merge or enable auto-merge >> until project CI has finished and passed. > > I will take a look from that description. This is all fixable - we have > **extreme** flexibility here and we can model our workflows as we want. We > are no longer held by what GitHub has - this is great benefit of AIP-120. > >> [^1] It currently lives at >> dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md - is that it’s >> final home? > > Any other proposals? This is not part of the AIP discussion - this is more of > a small implementation detail. Do we need it there at all? We already have https://github.com/apache/airflow/blob/main/contributing-docs/18_contribution_workflow.rst#ci-in-your-fork (Thanks Andrew for pointing that out).
