The PR with this "limited tests" PRs is ready for review https://github.com/apache/airflow/pull/11828
Not sure how much it's going to help before we have self-hosted runners, but it's worth trying. J. On Tue, Oct 27, 2020 at 8:51 AM Jarek Potiuk <[email protected]> wrote: > BTW. the Action from Tobiasz is already out there :) - he just adds the > comment/check option now: > https://github.com/TobKed/label-when-approved-action/ :D > > On Tue, Oct 27, 2020 at 8:27 AM Ash Berlin-Taylor <[email protected]> wrote: > >> 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 <[email protected]> >> 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 <[email protected]> wrote: >>> >>>> I am happy with "okay to test" / "run tests" . >>>> >>>> >>>> >>>> On Mon, Oct 26, 2020, 10:13 Jarek Potiuk <[email protected]> >>>> 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 <[email protected]> >>>>> 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 <[email protected]> >>>>>> wrote: >>>>>> >>>>>> what do you think about "Ready for review"? >>>>>> >>>>>> On Mon, Oct 26, 2020, 11:04 Jarek Potiuk <[email protected]> >>>>>> wrote: >>>>>> >>>>>> On Mon, Oct 26, 2020 at 10:53 AM Ash Berlin-Taylor <[email protected]> >>>>>> 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 < >>>>>> [email protected]> 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 < >>>>>> [email protected]> 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 < >>>>>> [email protected]> 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 < >>>>>> [email protected]> 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 < >>>>>> [email protected]> 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 < >>>>>> [email protected]> 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 < >>>>>> [email protected]> 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 >>>>>> [email protected] 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 <[email protected]> >>>>>> 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 < >>>>>> [email protected]> 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 <[email protected]> >>>>>> 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 < >>>>>> [email protected]> 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/> >>> >>> > > -- > > 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/>
