I created a PR for making this change: https://github.com/apache/pulsar/pull/9963 .
On Wed, Mar 17, 2021 at 6:23 PM Matteo Merli <matteo.me...@gmail.com> wrote: > +1 That was the behavior we used to have in Jenkins. > > > -- > Matteo Merli > <matteo.me...@gmail.com> > > On Wed, Mar 17, 2021 at 8:15 AM Michael Marshall <mikemars...@gmail.com> > wrote: > > > > +1 > > > > I like the idea that this change would give PRs the benefit of any > recently > > fixed flaky tests without needing to rebase. Rebasing to pick up fixes to > > flaky tests slows down development. > > > > Further, I think this sounds like a safer paradigm for maintaining a > > pristine master branch. > > > > Michael > > > > On Wed, Mar 17, 2021 at 2:01 AM Enrico Olivelli <eolive...@gmail.com> > wrote: > > > > > Il giorno mer 17 mar 2021 alle ore 08:56 Lari Hotari > > > <lari.hot...@sagire.fi> ha scritto: > > > > > > > > Dear Pulsar community members, > > > > > > > > I would like to propose a change how Pull Request (PR) builds are > handled > > > > in Pulsar CI. > > > > > > > > In GitHub Actions, the default way to handle PR builds is to checkout > > > > GitHub provided merge_commit_sha . > > > > > > > > GitHub's merge_commit_sha is explained in this documentation: > > > > https://docs.github.com/en/rest/reference/pulls#get-a-pull-request > > > > "Before merging a pull request, the merge_commit_sha attribute holds > the > > > > SHA of the test merge commit." > > > > Behind the scenes, GitHub automatically creates a "test merge commit" > > > that > > > > merges the PR branch and the target branch (master). A pull request > build > > > > won't be started at all if this test merge commit cannot be created > > > without > > > > conflicts. > > > > > > > > In Pulsar GitHub Actions workflows, the PR builds happen for the HEAD > > > > commit on the pull request branch, without a merge to the master > branch. > > > > > > > > *The downside of the approach used in the existing Pulsar CI is that > the > > > PR > > > > build won't pick up changes made in the master branch.* It is > required to > > > > explicitly rebase or merge the master branch to the pull request > branch > > > to > > > > get the changes. Another disadvantage is that there's a higher chance > > > that > > > > the PR breaks the master branch after the PR is merged. > > > > The isolated behavior might be seen as a benefit. The PR branch build > > > will > > > > be only impacted by the changes made in the PR branch. > > > > However, using this type of setting isn't *Continuous* Integration > (CI). > > > > > > > > The current custom build "diff-only" action ( > > > > https://github.com/apache/pulsar-test-infra/tree/master/diff-only) > > > doesn't > > > > support merge_commit_sha . When switching to use merge_commit_sha > instead > > > > of PR's head commit for git checkout, I'm suggesting to replace > diff-only > > > > action with https://github.com/dorny/paths-filter action . > paths-filter > > > > action doesn't have the limitations of diff-only action and uses > GitHub > > > API > > > > for resolving the changed files without a need to doing a checkout. > It > > > is a > > > > very high quality GitHub Action for adding conditional logic to > Github > > > > Action workflows based on the files that have changed. > > > > > > > > What do others think about making this change, using GitHub's > > > > merge_commit_sha for PR builds instead of PR's HEAD commit? > > > > > > +1 > > > using the PR HEAD commit is dangerous as we believe that the PR is not > > > breaking the target branch but currently we have no proof of that. > > > The risk is PRs that are far from the target branch. > > > When you merge a PR you don't have an easy way to check how far it is > > > from the target branch and you usually verify only that CI is green > > > and there are no conflicts > > > > > > Enrico > > > > > > > > > > > BR, Lari > > > >
