I would first try to not introduce the exception for local builds. It makes it quite hard for others to verify the build and to make sure that the right things were executed. If we see that this becomes an issue then we can revisit this idea.
Cheers, Till On Wed, Jun 23, 2021 at 4:19 AM Yangze Guo <karma...@gmail.com> wrote: > +1 for appending this to community guidelines for merging PRs. > > @Till Rohrmann > I agree that with this approach unstable tests will not block other > commit merges. However, it might be hard to prevent merging commits > that are related to those tests and should have been passed them. It's > true that this judgment can be made by the committers, but no one can > ensure the judgment is always precise and so that we have this > discussion thread. > > Regarding the unstable tests, how about adding another exception: > committers verify it in their local environment and comment in such > cases? > > Best, > Yangze Guo > > On Tue, Jun 22, 2021 at 8:23 PM 刘建刚 <liujiangangp...@gmail.com> wrote: > > > > It is a good principle to run all tests successfully with any change. > This > > means a lot for project's stability and development. I am big +1 for this > > proposal. > > > > Best > > liujiangang > > > > Till Rohrmann <trohrm...@apache.org> 于2021年6月22日周二 下午6:36写道： > > > > > One way to address the problem of regularly failing tests that block > > > merging of PRs is to disable the respective tests for the time being. > Of > > > course, the failing test then needs to be fixed. But at least that way > we > > > would not block everyone from making progress. > > > > > > Cheers, > > > Till > > > > > > On Tue, Jun 22, 2021 at 12:00 PM Arvid Heise <ar...@apache.org> wrote: > > > > > > > I think this is overall a good idea. So +1 from my side. > > > > However, I'd like to put a higher priority on infrastructure then, in > > > > particular docker image/artifact caches. > > > > > > > > On Tue, Jun 22, 2021 at 11:50 AM Till Rohrmann <trohrm...@apache.org > > > > > > wrote: > > > > > > > > > Thanks for bringing this topic to our attention Xintong. I think > your > > > > > proposal makes a lot of sense and we should follow it. It will > give us > > > > > confidence that our changes are working and it might be a good > > > incentive > > > > to > > > > > quickly fix build instabilities. Hence, +1. > > > > > > > > > > Cheers, > > > > > Till > > > > > > > > > > On Tue, Jun 22, 2021 at 11:12 AM Xintong Song < > tonysong...@gmail.com> > > > > > wrote: > > > > > > > > > > > Hi everyone, > > > > > > > > > > > > In the past a couple of weeks, I've observed several times that > PRs > > > are > > > > > > merged without a green light from the CI tests, where failure > cases > > > are > > > > > > considered *unrelated*. This may not always cause problems, but > would > > > > > > increase the chance of breaking our code base. In fact, it has > > > occurred > > > > > to > > > > > > me twice in the past few weeks that I had to revert a commit > which > > > > breaks > > > > > > the master branch due to this. > > > > > > > > > > > > I think it would be nicer to enforce a stricter rule, that no PRs > > > > should > > > > > be > > > > > > merged without passing CI. > > > > > > > > > > > > The problems of merging PRs with "unrelated" test failures are: > > > > > > - It's not always straightforward to tell whether a test > failures are > > > > > > related or not. > > > > > > - It prevents subsequent test cases from being executed, which > may > > > fail > > > > > > relating to the PR changes. > > > > > > > > > > > > To make things easier for the committers, the following > exceptions > > > > might > > > > > be > > > > > > considered acceptable. > > > > > > - The PR has passed CI in the contributor's personal workspace. > > > Please > > > > > post > > > > > > the link in such cases. > > > > > > - The CI tests have been triggered multiple times, on the same > > > commit, > > > > > and > > > > > > each stage has at least passed for once. Please also comment in > such > > > > > cases. > > > > > > > > > > > > If we all agree on this, I'd update the community guidelines for > > > > merging > > > > > > PRs wrt. this proposal. [1] > > > > > > > > > > > > Please let me know what do you think. > > > > > > > > > > > > Thank you~ > > > > > > > > > > > > Xintong Song > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/Merging+Pull+Requests > > > > > > > > > > > > > > > > > > >
