Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Fix CI Git logic #10316

Closed
wants to merge 11 commits into from
Closed

[WIP] Fix CI Git logic #10316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b658de7
Rename git test script
roryabraham Aug 8, 2022
ba0f32c
Update unit test to include failing scenario
roryabraham Aug 9, 2022
da7c16f
Fix failing test
roryabraham Aug 9, 2022
8f18743
Fix by using merges instead of cherry-pick
roryabraham Aug 9, 2022
2fc168a
Make utility methods to DRY up test
roryabraham Aug 9, 2022
f875089
Add more assertions and correct tests
roryabraham Aug 10, 2022
5d65ddb
Get tests passing
roryabraham Aug 10, 2022
657fb02
Use merge commits for merge-base
roryabraham Aug 10, 2022
f5686ff
Implement fix in cherryPick workflow
roryabraham Aug 10, 2022
a616fdc
Concise syntax for NEW_VERSION environment variable
roryabraham Aug 10, 2022
28eac20
Fix step output
roryabraham Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 72 additions & 72 deletions .github/workflows/cherryPick.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,23 +34,20 @@ jobs:
needs: [validateActor, createNewVersion]
if: ${{ always() && fromJSON(needs.validateActor.outputs.IS_DEPLOYER) }}
runs-on: ubuntu-latest
env:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more concise syntax for what used to be the Save correct NEW_VERSION to env step

NEW_VERSION: ${{ github.event.inputs.NEW_VERSION || needs.createNewVersion.outputs.NEW_VERSION }}
steps:
# Version: 2.3.4
- name: Checkout staging branch
uses: actions/checkout@5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f
with:
ref: staging
token: ${{ secrets.OS_BOTIFY_TOKEN }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to pass this token to try and set up git as OSBotify. I don't think it ever really worked, but in any event it's not necessary because in the next step we call setupGitForOSBotify.

fetch-depth: 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fetching all history so we don't need to do a separate fetch later


- uses: Expensify/App/.github/actions/composite/setupGitForOSBotify@main
with:
GPG_PASSPHRASE: ${{ secrets.LARGE_SECRET_PASSPHRASE }}

- name: Create branch for new pull request
run: |
git checkout -b ${{ github.actor }}-cherry-pick-staging-${{ github.event.inputs.PULL_REQUEST_NUMBER }}
git push --set-upstream origin ${{ github.actor }}-cherry-pick-staging-${{ github.event.inputs.PULL_REQUEST_NUMBER }}

- name: Get merge commit for CP pull request
id: getCPMergeCommit
uses: Expensify/App/.github/actions/javascript/getPullRequestDetails@main
Expand All @@ -59,18 +56,6 @@ jobs:
USER: ${{ github.actor }}
PULL_REQUEST_NUMBER: ${{ github.event.inputs.PULL_REQUEST_NUMBER }}

- name: Save correct NEW_VERSION to env
env:
NEW_VERSION: ${{ github.event.inputs.NEW_VERSION }}
run: |
if [ -z "$NEW_VERSION" ]; then
echo "NEW_VERSION=${{ needs.createNewVersion.outputs.NEW_VERSION }}" >> "$GITHUB_ENV"
echo "New version is ${{ env.NEW_VERSION }}"
else
echo "NEW_VERSION=${{ github.event.inputs.NEW_VERSION }}" >> "$GITHUB_ENV"
echo "New version is ${{ env.NEW_VERSION }}"
fi;

- name: Get merge commit for version-bump pull request
id: getVersionBumpMergeCommit
uses: Expensify/App/.github/actions/javascript/getPullRequestDetails@main
Expand All @@ -79,31 +64,38 @@ jobs:
USER: OSBotify
TITLE_REGEX: Update version to ${{ env.NEW_VERSION }}

- name: Cherry-pick the version-bump to new branch
- name: Create branch for new pull request
run: |
git fetch
git cherry-pick -S -x --mainline 1 --strategy=recursive -Xtheirs ${{ steps.getVersionBumpMergeCommit.outputs.MERGE_COMMIT_SHA }}
SOURCE_MERGE_BASE=$(git merge-base staging ${{ steps.getCPMergeCommit.outputs.MERGE_COMMIT_SHA }})
VERSION_BUMP_MERGE_BASE=$(git merge-base staging ${{ steps.getVersionBumpMergeCommit.outputs.MERGE_COMMIT_SHA }})
CP_MERGE_BASE=$(git merge-base "$SOURCE_MERGE_BASE" "$VERSION_BUMP_MERGE_BASE")
git checkout -b ${{ github.actor }}-cherry-pick-staging-${{ github.event.inputs.PULL_REQUEST_NUMBER }} "$CP_MERGE_BASE"
git push --set-upstream origin ${{ github.actor }}-cherry-pick-staging-${{ github.event.inputs.PULL_REQUEST_NUMBER }}

- name: Cherry-pick the merge commit of target PR to new branch
id: cherryPick
run: |
echo "Attempting to cherry-pick ${{ steps.getCPMergeCommit.outputs.MERGE_COMMIT_SHA }}"
if git cherry-pick -S -x --mainline 1 ${{ steps.getCPMergeCommit.outputs.MERGE_COMMIT_SHA }}; then
echo "🎉 No conflicts! CP was a success, PR can be automerged 🎉"
echo "::set-output name=SHOULD_AUTOMERGE::true"
else
echo "😞 PR can't be automerged, there are merge conflicts in the following files:"
git --no-pager diff --name-only --diff-filter=U
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a pretty weird workaround – we were literally committing the conflicts including the ====HEAD==== stuff, and asking people to overwrite that.

Fortunately, I don't think we'll really need to do this anymore. Instead, we'll CP the commits into a temporary branch which won't have any conflicts, then create the PR. If the PR has merge conflicts, then it will be unmergeable and we'll assign someone to resolve those conflicts. However, it will appear like a normal PR with merge conflicts listed at the bottom of the page. None of the wonky ====HEAD==== stuff will be committed into the files.

git add .
GIT_MERGE_AUTOEDIT=no git cherry-pick --continue
echo "::set-output name=SHOULD_AUTOMERGE::false"
fi
- name: Cherry-pick the version-bump to new branch
run: git cherry-pick -S -x --mainline 1 -Xtheirs ${{ steps.getVersionBumpMergeCommit.outputs.MERGE_COMMIT_SHA }}

- name: Cherry-pick the merge commit of target PR to the new branch
run: git cherry-pick -S -x --mainline 1 ${{ steps.getCPMergeCommit.outputs.MERGE_COMMIT_SHA }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to dig in to confirm, but I think there won't be conflicts against the temporary branch created from the merge-base of the two commits we're trying to CP.

I think the merge conflicts will pop up when we're attempting to merge code into staging (and even then, only if there are multiple conflicting CPs)


- name: Push changes to CP branch
run: git push

- name: Create Pull Request
id: createPullRequest
- name: Create main pull request
id: createMainPullRequest
run: |
gh pr create \
--title "🍒 Cherry pick PR #${{ github.event.inputs.PULL_REQUEST_NUMBER }} to main 🍒" \
--body "This pull request is needed so that this cherry-pick becomes a common ancestor for both main and staging. More details in https://github.com/Expensify/App/pull/10316" \
--label "automerge" \
--base "main"
sleep 5
echo "::set-output name=PR_NUMBER::$(gh pr view --json 'number' --jq '.number')"
env:
GITHUB_TOKEN: ${{ secrets.OS_BOTIFY_TOKEN }}

- name: Create staging pull request
id: createStagingPullRequest
run: |
gh pr create \
--title "🍒 Cherry pick PR #${{ github.event.inputs.PULL_REQUEST_NUMBER }} to staging 🍒" \
Expand All @@ -115,66 +107,74 @@ jobs:
env:
GITHUB_TOKEN: ${{ secrets.OS_BOTIFY_TOKEN }}

- name: Check if main pull request is mergeable
id: isMainPullRequestMergeable
uses: Expensify/App/.github/actions/javascript/isPullRequestMergeable@main
with:
GITHUB_TOKEN: ${{ github.token }}
PULL_REQUEST_NUMBER: ${{ steps.createMainPullRequest.outputs.PR_NUMBER }}

- name: Check if staging pull request is mergeable
id: isStagingPullRequestMergeable
uses: Expensify/App/.github/actions/javascript/isPullRequestMergeable@main
with:
GITHUB_TOKEN: ${{ github.token }}
PULL_REQUEST_NUMBER: ${{ steps.createStagingPullRequest.outputs.PR_NUMBER }}

- name: Check if ShortVersionString is up to date
id: isShortVersionStringUpdated
uses: Expensify/App/.github/actions/javascript/checkBundleVersionStringMatch@main

- name: Auto-assign PR if there are merge conflicts or if the bundle versions are mismatched
if: ${{ !fromJSON(steps.cherryPick.outputs.SHOULD_AUTOMERGE) || !fromJSON(steps.isShortVersionStringUpdated.outputs.BUNDLE_VERSIONS_MATCH) }}
run: gh pr edit --add-label "Engineering,Hourly"
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

- name: Assign the PR to the deployer
if: ${{ !fromJSON(steps.cherryPick.outputs.SHOULD_AUTOMERGE) }}
run: gh pr edit --add-assignee ${{ steps.getCPMergeCommit.outputs.MERGE_ACTOR }}
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- name: Check if the PRs can be auto-merged
id: checkForAutomerge
run: echo "::set-output name=SHOULD_AUTOMERGE::${{ fromJSON(steps.isMainPullRequestMergeable.outputs.IS_MERGEABLE) && fromJSON(steps.isStagingPullRequestMergeable.outputs.IS_MERGEABLE) && fromJSON(steps.isShortVersionStringUpdated.outputs.BUNDLE_VERSIONS_MATCH) }}"

- name: If PR has merge conflicts, comment with instructions for assignee
if: ${{ !fromJSON(steps.cherryPick.outputs.SHOULD_AUTOMERGE) }}
- name: Auto-assign PRs if either is not mergeable or if the bundle versions are mismatched
if: ${{ !fromJSON(steps.checkForAutomerge.outputs.SHOULD_AUTOMERGE) }}
run: |
gh pr comment --body \
"This pull request has merge conflicts and can not be automatically merged. :disappointed:
Please manually resolve the conflicts, push your changes, and then request another reviewer to review and merge.
**Important:** There may be conflicts that GitHub is not able to detect, so please _carefully_ review this pull request before approving."
gh pr edit ${{ steps.createMainPullRequest.outputs.PR_NUMBER }} --add-label "Engineering,Hourly"
gh pr edit ${{ steps.createStagingPullRequest.outputs.PR_NUMBER }} --add-label "Engineering,Hourly"
gh pr edit ${{ steps.createMainPullRequest.outputs.PR_NUMBER }} --add-assignee ${{ steps.getCPMergeCommit.outputs.MERGE_ACTOR }}
gh pr edit ${{ steps.createStagingPullRequest.outputs.PR_NUMBER }} --add-assignee ${{ steps.getCPMergeCommit.outputs.MERGE_ACTOR }}
gh pr comment ${{ steps.createMainPullRequest.outputs.PR_NUMBER }} --body \
"This pull request has an [associated mate against the staging branch](#${{ steps.createStagingPullRequest.outputs.PR_NUMBER }}) which had conflicts and could not be automatically merged. :disappointed:
Please manually resolve the conflicts, push your changes, and then request another reviewer to review and merge. _Only after doing that_, merge this PR."
gh pr comment ${{ steps.createStagingPullRequest.outputs.PR_NUMBER }} --body \
"This pull request has merge conflicts and can not be automatically merged. :disappointed:
Please manually resolve the conflicts, push your changes, and then request another reviewer to review and merge."
env:
GITHUB_TOKEN: ${{ secrets.OS_BOTIFY_TOKEN }}

- name: If PR has a bundle version mismatch, comment with the instructions for assignee
if: ${{ !fromJSON(steps.isShortVersionStringUpdated.outputs.BUNDLE_VERSIONS_MATCH) }}
run: |
gh pr comment --body \
"The CFBundleShortVersionString value in this PR is not compatible with the CFBundleVersion, so cherry picking it will result in an iOS deploy failure.
Please manually resolve the mismatch, push your changes, and then request another reviewer to review and merge.
**Important:** This mismatch can be caused by a failed Update Protected Branch workflow followed by a manual CP, but please confirm the cause of the mismatch before updating any version numbers."
gh pr comment ${{ steps.createStagingPullRequest.outputs.PR_NUMBER }} --body \
"The CFBundleShortVersionString value in this PR is not compatible with the CFBundleVersion, so cherry picking it will result in an iOS deploy failure.
Please manually resolve the mismatch, push your changes, and then request another reviewer to review and merge.
**Important:** This mismatch can be caused by a failed Update Protected Branch workflow followed by a manual CP, but please confirm the cause of the mismatch before updating any version numbers."
env:
GITHUB_TOKEN: ${{ secrets.OS_BOTIFY_TOKEN }}

- name: Auto-approve the PR
- name: Auto-approve the PRs
# Important: only auto-approve if there was no merge conflict!
if: ${{ fromJSON(steps.cherryPick.outputs.SHOULD_AUTOMERGE) }}
run: gh pr review --approve
if: ${{ fromJSON(steps.checkForAutomerge.outputs.SHOULD_AUTOMERGE) }}
run: |
gh pr review ${{ steps.createMainPullRequest.outputs.PR_NUMBER }} --approve
gh pr review ${{ steps.createStagingPullRequest.outputs.PR_NUMBER }} --approve
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

- name: Check if pull request is mergeable
id: isPullRequestMergeable
uses: Expensify/App/.github/actions/javascript/isPullRequestMergeable@main
with:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
PULL_REQUEST_NUMBER: ${{ steps.createPullRequest.outputs.pr_number }}

- name: Auto-merge the PR
# Important: only auto-merge if there was no merge conflict and the PR is mergable (not blocked by a missing status check)!
if: ${{ fromJSON(steps.cherryPick.outputs.SHOULD_AUTOMERGE) && fromJSON(steps.isPullRequestMergeable.outputs.IS_MERGEABLE) }}
run: gh pr merge ${{ steps.createPullRequest.outputs.pr_number }} --merge --delete-branch
if: ${{ fromJSON(steps.checkForAutomerge.outputs.SHOULD_AUTOMERGE) }}
run: |
gh pr merge ${{ steps.createMainPullRequest.outputs.PR_NUMBER }} --merge --delete-branch
gh pr merge ${{ steps.createStagingPullRequest.outputs.PR_NUMBER }} --merge --delete-branch
env:
GITHUB_TOKEN: ${{ secrets.OS_BOTIFY_TOKEN }}

- name: 'Announces a CP failure in the #announce Slack room'
uses: 8398a7/action-slack@v3
if: ${{ failure() || !fromJSON(steps.isPullRequestMergeable.outputs.IS_MERGEABLE) }}
if: ${{ failure() || !fromJSON(steps.checkForAutomerge.outputs.SHOULD_AUTOMERGE) }}
with:
status: custom
custom_payload: |
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,5 @@ jobs:
env:
CI: true

- name: Pull Request Tests
run: tests/unit/getPullRequestsMergedBetweenTest.sh
- name: Test CI Git Logic
run: tests/unit/gitTestCI.sh
27 changes: 27 additions & 0 deletions scripts/shellUtils.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,30 @@ function assert_equal {
success "Assertion passed: $1 is equal to $1"
fi
}

function assert_string_contains_substring {
if [[ "$1" != *"$2"* ]]; then
error "Assertion failed: \"$1\" does not contain substring \"$2\""
exit 1
else
success "Assertion passed: \"$1\" contains substring \"$2\""
fi
}

function assert_string_doesnt_contain_substring {
if [[ "$1" == *"$2"* ]]; then
error "Assertion failed: \"$1\" contains substring \"$2\""
exit 1
else
success "Assertion passed: \"$1\" does not contain substring \"$2\""
fi
}

function assert_file_doesnt_exist {
if [[ -f "$1" ]]; then
error "Assertion failed: File $1 exists"
exit 1
else
success "Assertion passed: File $1 does not exist"
fi
}
Loading