> Now that we aren’t using ASF runners anyway, what is the driver for this
> change? Are we worried about how quickly we are using our AWS credits?
>
We only use AWS credits for canary builds and committer builds.
>
> More fundamentally: I don’t get what problem we are actually hoping to
> solve with this change?
>
* making it harder for external contributors to submit big PRs
* be a good citizen for ASF - and help both us and other projects to have
better CI experience
* this means that we help others in our community. Several ASF projects
already helped us by doing the same and freeing the queue in similar way,
so it makes sense we do the same - if only as being kind and return the
favour to others. I know not all people care about "others" - mostly about
"self" - but I do. Deeply. And I hope when they care about us - it makes
sense to care about them.
* as Shahar explained - the shared resources will get limited likely - due
to really high demand
And also, the bigger question above, -1 to the flow **as it stands today**
> (even assuming the 15min fork reconciler is moved to a quicker interval
> later). This flow feels clunky, and until my above question is answered
> clearly and succinctly, I do not see the value in this change.
>
>
Let's work on those changes. I will apply comments from the thread and
improve things
I am happy to cancel the vote now and restart it with clarifications and by
addressing all the comments. and discussion. Earlier discussion did not
spark many comments, so I am happy we have them now.
Some of these may be fixable, but here are the things I don’t like about
> this flow:
>
> - 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,
It's impossible for a fork workflow to post status; they cannot have write
permissions to our PRs.
- 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.
> 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.
>
>
> Doubly so as once it is put in to draft, it stays there. It didn’t get
> automatically marked as not draft for me. Maybe I didn’t wait long enough?
> This PR stayed in draft for the whole fork run, which was about 45 minutes
> here, plus up to one reconciler cycle, so roughly an hour from push to
> ready.
>
Again - that was a bug. We discussed all kinds of bugs in Slack, and we
continue to do so. Post any issues like that in #internal-airlfow-ci-cd
-and we will fix it - there is a whole team of people working on it now
(and a bunch of agents).
>
> - No granular feedback report, or even obvious link to view the actions
> runs on the fork.
>
This was removed at some point in time -> it worked like that, you actually
have granular feedback, but it only shows when the job fails or gets
cancelled. The idea was that you are really interested in failures not -
individual successes. But we can work on that and be very flexible there.
> - 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".
Once fork CI is green, the reconciler posts the required "CI passed"
> status as success.
>
No - it's gated by the above.
> Adding "use project ci" never withdraws that success state, and
> doesn't start project CI either; for that you have to close and reopen the
> PR (or push).
>
We do have "label listening" workflow - > we can fix any issues there if we
get an example of that. See above. Just post those examples in the channel.
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.
>
>
> My tl:dr: this makes our CI more complex. And I don’t see the complexity
> cost outweighs the benefits.
>
Yes. It does. This is the price to pay for being good citizen. And
complexity costs had gone significantly down with AI. I would agree a year
ago cost would outweight the benefits. But not today. Those costs approach
0 and we should change our thinking about "tooling complexity" being a
problem - if it solves others problems.
>
> [^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.
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>
>
