Merge PR to its destination branch before parsing CI config and executing build

[yogurtearl]

Description

Add a (default) option to checkout and merge a branch to its destination branch for PR builds before you parse the .cirrus.yml

After the PR has been merged to its destination branch, then you parse the .cirrus.yml and do whatever executions need to be done.

Context

The goal of a PR build is to make sure that merging the PR doesn't break anything. Merging to the destination branch before parsing the .cirrus.yml makes the PR build closer to what the post merge build will be.

Merging before you parse the .cirrus.yml because the unmerged .cirrus.yml on a PR might reference things that are deleted or changed after the merge.

Also, if the docker images are changed in the destination branch, you will want to make sure builds can still succeed inside the new images.

Anything Else

Ideally this would be the default with an option to opt out.

Many other CI systems have a default behavior similar to this:

• You can turn this off in GH actions: https://github.com/actions/checkout#Checkout-pull-request-HEAD-commit-instead-of-merge-commit
• Travis CI: https://docs.travis-ci.com/user/pull-requests/#how-pull-requests-are-built
• Bitrise: https://medium.com/@bitrise/git-clone-step-revised-on-bitrise-more-stable-and-more-reliable-ddc2e043607f?_branch_match_id=781735949155432941

[Minoru]

I believe this can be achieved by setting "Config resolution strategy" to "Merge for PRs". This is not documented yet, but https://github.com/cirruslabs/cirrus-ci-docs/issues/662#issuecomment-742832180 has an explanation of all the options.

[fkorotkov]

@yogurtearl the only caveat here is that Cirrus agent won't clone a merged version of the branch. I've been debating whether make the agent automatically clone merged version of the sources if resolution strategy set to Merge for PRs or introduce a new behavior environment variable that will control it.

[maflcko]

I think it is fine to clone the merged version of the branch when Merge for PRs is set. There shouldn't be a use-case to merge the config yaml, but keep the outdated repo/code/other config.

Otherwise this makes the dockerfile setup more brittle. A common workaround for this bug (and discussion https://github.com/cirruslabs/cirrus-ci-docs/discussions/1110) is to add a explicit merge step to the cirrus yaml. However, this doesn't work for dockerfile builds, because the image is built before any steps are executed.

[maflcko]

I am happy to work on a fix for this, but I don't know how to extract the Merge for PRs setting value in the function that needs it ( https://github.com/cirruslabs/cirrus-ci-agent/blob/6d73a3e0e801133ce013491fb28f4b1745e119f6/internal/executor/executor.go#L575 ) ?

[maflcko]

Looks like there were two merged pull requests, so I wonder if this was already fixed or if more work is needed?

[edigaryev]

Looks like there were two merged pull requests, so I wonder if this was already fixed or if more work is needed?

This is indeed already fixed, thanks for the nudge!

Note that the changes only improve the workflow for the Merge for PRs config resolution strategy, the Latest from default branch resolution strategy requires merging the changes against the default branch, but this functionality is not yet available on GitHub.

[real-or-random]

Thanks for the fixes.

I'm not entire sure if it solves the problem entirely. For example here, the log message indicates that the checked out commit is b160ddd00f9ccc37696b0fdf268d76ad85146453, which is still on the PR branch. In other words, it's not the merge commit that is checked out. And we would still need our manual merge_base script (see right below "Clone").

[edigaryev]

For example here, the log message indicates that the checked out commit is b160ddd00f9ccc37696b0fdf268d76ad85146453, which is still on the PR branch. In other words, it's not the merge commit that is checked out.

Thanks for noticing this!

The "Merge for PRs" config resolution strategy enabled, skipping hard reset to preserve the merge commit line indicates that the new logic works, you can check it by running git log: the latest commit should be the merge commit, not b160ddd00f9ccc37696b0fdf268d76ad85146453.

The Checked out b160ddd00f9ccc37696b0fdf268d76ad85146453 on pull/1329 branch message is indeed misleading, will improve it in the agent.