-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Description
The current CI system builds a slightly different set of sources in GitHub than the one in PR directly. But possibly it is better. We can fix it with a workaround but I am not sure if we should.
Details:
-
when we currently build the image in "Build Image" workflow we are using "scripts/ci" from master and rest of the sources from the commit that is the HEAD of PR.
-
However the sources in the PR run by GitHub are slightly different. When there are no conflicts, Github actually performs a merge between the master and the PR and the sources in build PR are those merged sources. But this only happens when there is no conflict, otherwise, original sources are used.
This is not a big issue IMHO now and I'd argue that it's better if we run original sources, because that might be a source of confusion if someone tries to reproduce it locally. What you have to do to replicate the PR build, you have to rebase it locally on top of latest master. But this is not at all clear - I just learned that this is happening while implementing the new CI and had no idea that this was happening - and it could have explained a number of "We do not know what happened in this CI" cases.
I can workaround this (there's no API in Github to know what is the merge request Commit SHA, but I Already use similar workarounds passing information via job names (waiting for missing API from GitHub). But I am not sure if it is worth it.
For now it might cause problems similar to the ones in #10445 but a fix is coming in #10472 so that such failures will not happen but we will use the HEAD of PR as commit SHA still. Possibly also #10806
Another problem that was caused by this was https://github.com/apache/airflow/runs/1108600218?check_suite_focus=true resulting from merging #10898. In this PR new packages were added, and later not rebased PR which has been just merged, and the PR #10906 which was not rebased on top of it resulted in Pylint being run on "merged" sources with an image that did not contain the "merged" dependencies.
@kaxil @feluelle @turbaszek @mik-laj @dimberman - WDYT.
BTW. We also have a safety net. The build with "merged" sources happens always after we merge the PR anyway - so in case a problem is hidden we will see it failing at "push" event.