+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 >
