I think having a test/check status they shows in progress until approved is actually a good thing - it makes it more explicit that there are more tests to come.
On 27 October 2020 07:22:00 GMT, Jarek Potiuk <jarek.pot...@polidea.com> wrote: >We are close to the finish, but we've hit some GH limitations with >Tobiasz. >It turned out that the re-run workflow API ( >https://docs.github.com/en/free-pro-team@latest/rest/reference/actions#re-run-a-workflow) >has an undocumented feature :) - it only allows to re-run failed runs, >but >it does not work on successful ones. This only works for manual re-runs >from the UI, but not via API. This is a requested feature ( >https://github.community/t/is-it-possible-to-manually-force-an-action-workflow-to-be-re-run/2127/22) >but we cannot wait for it. > >We thought about it and slept over it and since we cannot wait for it >we >thought about a bit different approach which we are implementing: > >When PR gets its approval, it will automatically get the "okay to test" >label and a comment inviting to rebasing the PR or re-running the tests >and >explaining why. > >We will also experiment with adding an extra "check" that will mark the >PR >as still "in-progress" in this case so that it is obvious that the PR >is >not yet "completely" tested. Later we will skip all that for the >doc-only >PRs that do not require tests at all. > >Let us know if you have any thoughts about it. > >J, > >On Mon, Oct 26, 2020 at 12:28 PM Kaxil Naik <kaxiln...@gmail.com> >wrote: > >> I am happy with "okay to test" / "run tests" . >> >> >> >> On Mon, Oct 26, 2020, 10:13 Jarek Potiuk <jarek.pot...@polidea.com> >wrote: >> >>> Kamil - "Ready for review" is not good - it must have been reviewed >>> already because it has at least one approval. >>> >>> Ash - I am ok with "okay to test" :). Hard to mistake it with >>> anything else and serves the purpose well :) >>> >>> Any other opinions/voices :)? I already have the PRs to enable it in >>> review, and we work with Tobiasz on auto-labeling action so >hopefully >>> today/tomorrow we can get it up and running. >>> >>> J >>> >>> >>> On Mon, Oct 26, 2020 at 11:08 AM Ash Berlin-Taylor <a...@apache.org> >>> wrote: >>> >>>> How about "okay to test" -- that's often the "command" that people >use >>>> for test approval (thinking of Jenkins Github integration, where >you can >>>> say "ready to test" to do this exact purpose). >>>> >>>> -ash >>>> >>>> On Oct 26 2020, at 10:06 am, Kamil Breguła ><kamil.breg...@polidea.com> >>>> wrote: >>>> >>>> what do you think about "Ready for review"? >>>> >>>> On Mon, Oct 26, 2020, 11:04 Jarek Potiuk <jarek.pot...@polidea.com> >>>> wrote: >>>> >>>> On Mon, Oct 26, 2020 at 10:53 AM Ash Berlin-Taylor <a...@apache.org> >>>> wrote: >>>> >>>> Is "ready to merge" also going to automatically merge if tests are >green? >>>> >>>> >>>> Not at all. It was never the intention. Committers still need to >merge >>>> it manually. The difference is that you will see the "Ready to >Marge" label >>>> and "green" (hopefully) merge button, you will know that the "full >set" of >>>> tests was successful. >>>> >>>> I am also not sure if "Ready to Merge" is best name though. I've >been >>>> thinking about this but think it could be simply "All test", "Full >test >>>> set" ... or simply maybe "Ready for all tests"*.* >>>> >>>> I think the last one is best ("Ready for all tests") >>>> >>>> J. >>>> >>>> >>>> >>>> I think it shouldn't unless we also remove that label on a new push >to >>>> the branch - consider this: >>>> >>>> - PR is reviewed and approved and a simple change, committer >reviews >>>> it and gives it an approval; tests currently running >>>> >>>> >>>> - Label is applied >>>> - While tests are running PR author pushes malicious code >>>> - Tests for this new push pass and it's automatically merged. >>>> >>>> >>>> Because of this I think "ready to merge" is actually the wrong name >as >>>> it conveys extra meaning that we want to avoid. (And I also don't >want to >>>> remove approvals when pushing, there are many many cases where it's >just a >>>> small change requested, and we give approval with "make this >change; I'm >>>> pre-emptively approving it") >>>> >>>> -ash >>>> >>>> On Oct 24 2020, at 8:49 pm, Daniel Imberman ><daniel.imber...@gmail.com> >>>> wrote: >>>> >>>> I think ready to merge makes more sense >>>> >>>> via Newton Mail >>>> ><https://cloudmagic.com/k/d/mailapp?ct=dx&cv=10.0.51&pv=10.15.6&source=email_footer_2> >>>> >>>> On Sat, Oct 24, 2020 at 12:13 PM, Jarek Potiuk ><jarek.pot...@polidea.com> >>>> wrote: >>>> >>>> Some short update - seems like we can get 50% 60% saving in job >usage by >>>> the "unapproved PRs". We are progressing with implementation :D. >>>> >>>> On Sat, Oct 24, 2020 at 10:55 AM Jarek Potiuk ><jarek.pot...@polidea.com> >>>> wrote: >>>> >>>> FYI. We found out with Tobiasz, that it will be a bit better and if >we >>>> add "*Approved*" label in the PR that the workflow will >automatically >>>> set when the issue gets approved. >>>> >>>> This way we have state of the PR approval (we know when it changes) >and >>>> know that we should re-run last "small matrix" successful run when >the >>>> label changes to "Approved". >>>> >>>> This will also be an additional indication to committers in case of >>>> queues and delays we se. It might be that the "small" matrix run is >already >>>> successful, the PR gets approved but the "full matrix build" is >delayed due >>>> to queuing. Such PR will have green "merge" button and might get >merged by >>>> mistake - but it will not have the "Approved" label yet. Setting >the label >>>> and re-running the build will happen at the same time. >>>> >>>> But I start thinking this label should be named differently - how >about "*Ready >>>> to merge*" maybe? Or maybe other ideas? >>>> >>>> J. >>>> >>>> >>>> >>>> >>>> On Sat, Oct 24, 2020 at 1:10 AM Daniel Imberman < >>>> daniel.imber...@gmail.com> wrote: >>>> >>>> +1000! >>>> >>>> via Newton Mail >>>> ><https://cloudmagic.com/k/d/mailapp?ct=dx&cv=10.0.51&pv=10.15.6&source=email_footer_2> >>>> >>>> On Fri, Oct 23, 2020 at 8:22 AM, Jarek Potiuk ><jarek.pot...@polidea.com> >>>> wrote: >>>> >>>> Thanks Tobiasz :). fantastic. >>>> >>>> I prepared a very short and simple design doc >>>> >https://docs.google.com/document/d/16rwyCfyDpKWN-DrLYbhjU0B1D58T1RFYan5ltmw4DQg/edit# >where >>>> we can collaborate. >>>> >>>> I also added you as collaborator to >>>> https://github.com/potiuk/get-workflow-origin that we already use, >and >>>> I think you can update the "get workflow origin" plugin to include >status >>>> of the PR in the output of the action (ore maybe we find out that >we >>>> already have what we need in GitHub context). >>>> >>>> I will take a look at finding out how/if we can trigger the "full >build" >>>> automatically when approval status changes from "Not approved" to >>>> "Approved". >>>> >>>> J. >>>> >>>> >>>> >>>> >>>> On Thu, Oct 22, 2020 at 7:20 PM Tobiasz Kędzierski < >>>> tobiasz.kedzier...@polidea.com> wrote: >>>> >>>> Hi Jarek. >>>> >>>> sounds good to me. I am happy to help you as much as I can with it. >>>> >>>> BR >>>> Tobiasz >>>> >>>> On Thu, Oct 22, 2020 at 9:06 AM Jarek Potiuk ><jarek.pot...@polidea.com> >>>> wrote: >>>> >>>> *TLDR; I thought about it a bit and I have a proposal on how to >solve it >>>> even better - one that can be implemented over the weekend (I >volunteer :) >>>> ) and that can be very easily shared and adopted by the other ASF >projects >>>> so that we all collectively decrease the strain on Github Actions.* >>>> >>>> This is in parallel to our efforts on having self-hosted workers of >>>> course, but I think it will be needed anyway. Let me put it in a >bit of >>>> context >>>> >>>> *Problem statement:* >>>> >>>> * the root cause of the problem is that we are competing with many >other >>>> projects of ASF for the 180 jobs. I have started the discussion in >>>> bui...@apache.org about this: >>>> >https://lists.apache.org/thread.html/r1708881f52adbdae722afb8fea16b23325b739b254b60890e72375e1%40%3Cbuilds.apache.org%3E >and >>>> it's clear all ASF projects using GA have the same problem and >compete >>>> against each other for the jobs. >>>> * if we decrease the strain on our side, this is not solving the >problem >>>> long term. We keep on doing it already, and we already decrease a >lot of >>>> strain, but other projects from ASF increase their strain in the >meantime >>>> (Apache Beam, Skywalking, and few other projects are becoming heavy >GitHub >>>> Actions users). >>>> * in all the projects that I looked at, we have the same root >cause. >>>> Matrix strategy of builds causes enormous strain on Github Actions >if the >>>> whole matrix is run for all PRs. We are going to make it works >sustainably >>>> only if we come up with an easy solution, that can be applied to >all those >>>> projects. >>>> * I think the comment-based PR triggering process is complex and >>>> cumbersome to follow. It puts a LOT more effort on the committers >because >>>> they not only have to review and comment on the PRs but also make >decisions >>>> that those PRs are ready for "full build". This is a lot of >unnecessary >>>> effort and complicated process that many of the ASF projects will >not like >>>> to adopt >>>> >>>> *Proposed Solution:* >>>> >>>> *Add an easy way to limit the matrix strategy to one "default" >combo for >>>> PRs THAT HAVE NOT BEEN APPROVED BY COMMITTERS YET* >>>> >>>> This can be easily done with Github Actions workflows - no need to >write >>>> a bot for this. >>>> >>>> *Some details:* >>>> >>>> * Custom GitHub Action (generic) that checks if the PRs are >approved by >>>> Committers (and no "disapprovals"). The action would produce an >output -> >>>> "Approved", "Not approved". The output could be used to determine >the >>>> matrix strategy scope (in our case we already have support for >dynamic >>>> matrix strategy that I added a few weeks ago - so it's just a >matter of >>>> wiring the output in). >>>> * Very small workflow with the same GithubAction run on >>>> "pull_request_target" event. That workflow would effective >"observe" the >>>> PR, and when the status changes from "not approved" to "approved", >it >>>> triggers a PR build (with the "full matrix strategy" this time >because the >>>> PR will be already approved). This seems to be entirely possible. >This >>>> "pull_request_target" workflow - similarly to "workflow_run" runs >with a >>>> "write" access token and uses a "main branch" workflow version and >it could >>>> easily trigger a rerun of the last PR build in such case. >>>> >>>> *Benefits:* >>>> >>>> - I think I could write such an action over the coming weekend >(happy to >>>> collaborate with anyone on that). I will first search if someone >has done >>>> something similar of course because maybe it can be done faster >this way, >>>> but I am quite confident after writing my >>>> https://github.com/potiuk/cancel-workflow-runs which we already use >to >>>> limit the strain that it is doable in a day/two >>>> - no need to change the process we have - we continue working as we >did >>>> and simply "approved" PRs will be the full matrix strategy ones but >the >>>> "not-approved-ones" will run a limited version of the checks. >>>> - no way to accidentally submit a breaking PR - when the committer >>>> approves the PR that has not been approved before, the PR build >will be >>>> re-run with the "full matrix strategy" and not mergeable until it >finishes >>>> - last-but-not-least: we can propose (and help) other ASF projects >to >>>> use the action in their own GitHub Actions. It will not be changing >>>> anyone's process - which makes it super-easy to adopt and I can >even turn >>>> it into a "recommended solution" by Apache Infra - similarly as >Airflow's >>>> CI architecture is a recommended solution already for the >integration of GA >>>> with DockerHub >>>> >https://cwiki.apache.org/confluence/display/INFRA/Github+Actions+to+DockerHub >>>> >>>> WDYT? >>>> >>>> J >>>> >>>> >>>> >>>> On Fri, Oct 2, 2020 at 7:59 PM Jarek Potiuk ><jarek.pot...@polidea.com> >>>> wrote: >>>> >>>> I think it's a good idea, but I'd augment it a bit. A better option >will >>>> be to run all test types but >>>> for only one chosen combination of "Python + DB type + DB version". >>>> >>>> I often don't even look at the PR until the tests pass and this >would be >>>> much better this way. >>>> And often people have slower/small machines so they submit the PR >to see >>>> if they have not >>>> broken any other tests. This is much, much easier than doing it >locally >>>> - because then in one >>>> "fire&forget" you can run static, doc, unit tests, integration, and >>>> Kubernetes ones,. And it's a valid >>>> thing. >>>> >>>> Also, we have to make sure that such PR does not become "Green" >before >>>> all the tests are run. >>>> This might be rather problematic as Github does not yet have a " >manual" >>>> Approval step in >>>> Github Actions (it's coming in Q4: >>>> https://github.com/github/roadmap/issues/99). >>>> >>>> We have many tests and already we hit a bug a few times, where not >all >>>> tests have yet started >>>> and we've merged such PR. I can imagine it will happen more and >more >>>> often if all PRs will >>>> only run a subset of tests. It will be very easy to make that >mistake >>>> because even if we run a subset of >>>> those tests, we have so many jobs that you cannot see them all in >the >>>> GitHub UI. >>>> >>>> So we will have to have a check that fails the PR but marks it >somehow >>>> as "Ready for review" for example adding >>>> a label "Ready for review" when the subset of tests succeeds. >>>> >>>> Also, this might not be needed (or less important) if we implement: >>>> https://github.com/apache/airflow/issues/10507 "Selective Tests" >>>> for which I have an open PR. They will give much bigger >improvements - >>>> because, in the vast majority of cases, the tests will take >>>> very little time - giving feedback >>>> about relevant tests in a few minutes rather than half-an-hour. We >can >>>> also combine those two. >>>> >>>> It seems that I managed to finish some of the stuff that I thought >will >>>> take more time, so I might come back to it next week >>>> if it goes as well as I planned. >>>> >>>> J. >>>> >>>> >>>> >>>> On Fri, Oct 2, 2020 at 3:57 AM Daniel Imberman < >>>> daniel.imber...@gmail.com> wrote: >>>> >>>> I’m not too worried about that. I think people would learn pretty >>>> quickly. It hasn’t been an issue for the kubernetes community so I >can’t >>>> imagine it being an issue for us. End-of-day, we only have a >limited amount >>>> of compute power and this will increase the speed we merge the PR’s >that >>>> have passed basic code quality checks. >>>> >>>> >>>> On Thu, Oct 1, 2020 at 2:19 PM, Tomasz Urbaszek ><turbas...@apache.org> >>>> wrote: >>>> >>>> I think I can agree. Especially with flaky tests, some contributors >may >>>> be confused that some of the tests don't work on CI but work >locally... >>>> >>>> Checking the code quality is good first step. Once there's a review >we >>>> can start tests on CI. >>>> >>>> On the other hand, I can see people asking for starting the tests >or >>>> being even more confused why some PRs have more CI builds than >others... >>>> >>>> Cheers, >>>> Tomek >>>> >>>> On Thu, Oct 1, 2020 at 10:29 PM Daniel Imberman < >>>> daniel.imber...@gmail.com> wrote: >>>> >>>> Hello all, >>>> >>>> With the recent uptick in airflow contribution and pull requests, I >have >>>> a proposal that I hope will ensure that we do not find ourselves in >a CI >>>> backlog hell. I noticed that on the Kubernetes project, pull >requests do >>>> not run integration test until a committer submits a "ready to >test" >>>> command to the CI bot. This step can prevent draft PRs or >un-reviewed PRs >>>> from taking github CI resources. It is worth noting that with >breeze's >>>> docker based testing system, users have the exact same testing >capabilities >>>> locally as they would on our CI. >>>> >>>> I propose that we allow unverified PR's to run basic and static >tests, >>>> but not perform the full test suite or integration test without >first being >>>> reviewed. >>>> >>>> What does everyone think? >>>> >>>> >>>> >>>> -- >>>> >>>> Jarek Potiuk >>>> Polidea <https://www.polidea.com/> | Principal Software Engineer >>>> >>>> M: +48 660 796 129 <+48660796129> >>>> [image: Polidea] <https://www.polidea.com/> >>>> >>>> >>>> >>>> -- >>>> >>>> Jarek Potiuk >>>> Polidea <https://www.polidea.com/> | Principal Software Engineer >>>> >>>> M: +48 660 796 129 <+48660796129> >>>> [image: Polidea] <https://www.polidea.com/> >>>> >>>> >>>> >>>> -- >>>> >>>> Jarek Potiuk >>>> Polidea <https://www.polidea.com/> | Principal Software Engineer >>>> >>>> M: +48 660 796 129 <+48660796129> >>>> [image: Polidea] <https://www.polidea.com/> >>>> >>>> >>>> >>>> -- >>>> >>>> Jarek Potiuk >>>> Polidea <https://www.polidea.com/> | Principal Software Engineer >>>> >>>> M: +48 660 796 129 <+48660796129> >>>> [image: Polidea] <https://www.polidea.com/> >>>> >>>> >>>> >>>> -- >>>> >>>> Jarek Potiuk >>>> Polidea <https://www.polidea.com/> | Principal Software Engineer >>>> >>>> M: +48 660 796 129 <+48660796129> >>>> [image: Polidea] <https://www.polidea.com/> >>>> >>>> >>>> >>>> -- >>>> >>>> Jarek Potiuk >>>> Polidea <https://www.polidea.com/> | Principal Software Engineer >>>> >>>> M: +48 660 796 129 <+48660796129> >>>> [image: Polidea] <https://www.polidea.com/> >>>> >>>> >>> >>> -- >>> >>> Jarek Potiuk >>> Polidea <https://www.polidea.com/> | Principal Software Engineer >>> >>> M: +48 660 796 129 <+48660796129> >>> [image: Polidea] <https://www.polidea.com/> >>> >>> > >-- > >Jarek Potiuk >Polidea <https://www.polidea.com/> | Principal Software Engineer > >M: +48 660 796 129 <+48660796129> >[image: Polidea] <https://www.polidea.com/>
