You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/08/22 07:58:35 UTC
[GitHub] [airflow] potiuk opened a new issue #10471: Current CI builds slightly different sources than the one in PR build
potiuk opened a new issue #10471:
URL: https://github.com/apache/airflow/issues/10471
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:
1) 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.
2) 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 shortly so that such failures will not happen but we will use the HEAD of PR as commit SHA still.
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on issue #10471: Current CI builds slightly different sources than the one in PR build
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #10471:
URL: https://github.com/apache/airflow/issues/10471#issuecomment-693291965
> This is not a big issue IMHO now and I'd argue that it's better if we run original sources
More predictable, perhaps, but less useful overall: By running against the result of merging master and the PR, the tests effectively test "will this work when merged to master" which is more important than just "doe the tests pass on the branch".
By only testing the PR directly we're more likely to have broken master builds that we don't notice straight away (I don't know about anyone else, but when I merge a PR I don't even think about checking the build status on master 20-40mins later when it runs.), so changing to only test PR, not the merge result, would lead to more work needing to track down when the build started failing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #10471: Current CI builds slightly different sources than the one in PR build
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #10471:
URL: https://github.com/apache/airflow/issues/10471#issuecomment-691692774
For example missing dependencies that has just been added. In any case, the workaround is simply to rebase such PR to the latest master, but I think I saw several of those happening recently, pretty much always when we add new dependencies, so it might be worth fixing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on issue #10471: Current CI builds slightly different sources than the one in PR build
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #10471:
URL: https://github.com/apache/airflow/issues/10471#issuecomment-693290142
> when we currently build the image in "Build Image" workflow we are using "scripts/ci" from master
I haven't yet had a chance to look in to this new Build Image workflow, but is it not possible to have this run as a second workflow file on PRs, rather than running it against master?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed issue #10471: Current CI builds slightly different sources than the one in PR build
Posted by GitBox <gi...@apache.org>.
potiuk closed issue #10471:
URL: https://github.com/apache/airflow/issues/10471
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #10471: Current CI builds slightly different sources than the one in PR build
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #10471:
URL: https://github.com/apache/airflow/issues/10471#issuecomment-691692568
I might want to try to fix that one if we have more problems like this.. The problems will manifest in sometimes weird errors if the PR has not been rebased to the latest master.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #10471: Current CI builds slightly different sources than the one in PR build
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #10471:
URL: https://github.com/apache/airflow/issues/10471#issuecomment-693380278
Yeah - agree it is less useful. Looking at the number of issues it caused - they are not very big, but looking at how difficult it is to diagnose them and explain them to users, I am also for doing what GitHub is doing by default (i.e. merging). I will take a look at how I can implement it in an easiest and robust way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org