danielle-casella-adams
commented
2 years ago @hpractv I dismissed your review by pushing the change I started pushing yesterday but I didn't hit the one last dialog box to push it up.
Closed danielle-casella-adams closed 2 years ago
danielle-casella-adams
commented
2 years ago @hpractv I dismissed your review by pushing the change I started pushing yesterday but I didn't hit the one last dialog box to push it up.
mike-schenk
commented
2 years ago @danielle-casella-adams
From: https://docs.github.com/en/rest/reference/pulls#get-a-pull-request
When you get, create, or edit a pull request, GitHub creates a merge commit to test whether the pull request can be automatically merged into the base branch. This test commit is not added to the base branch or the head branch.
... that's why you don't see it in any of your logs.
The value of the
merge_commit_shaattribute changes depending on the state of the pull request. _Before merging a pull request, themerge_commit_shaattribute holds the SHA of the test merge commit._ After merging a pull request, themerge_commit_shaattribute changes depending on how you merged the pull request:
- If merged as a merge commit,
merge_commit_sharepresents the SHA of the merge commit.- If merged via a squash,
merge_commit_sharepresents the SHA of the squashed commit on the base branch.- If rebased,
merge_commit_sharepresents the commit that the base branch was updated to.
(emphasis added)
The context.sha property is set from the GITHUB_SHA environment variable whose value is apparently set by the pull request trigger from the pull request's merge_commit_sha attribute.
The pull_request trigger documentation that says GITHUB_SHA is the "Last merge commit on the GITHUB_REF branch" and that GITHUB_REF is the "PR merge branch" isn't exactly clear about this. It's subtle that the "PR merge branch" is neither the PR's base branch, nor its head branch. It's refs/pull/:prNumber/merge... that's the branch where that trial merge commit lives.
Note that the default behavior of actions/checkout will still checkout the trial merge commit, so unless the workflow specifies to checkout the PR's head branch, this change in this PR will end up tagging a different commit than the one that supplied the source code that was built. When a PR's branch is already up-to-date with its base branch, this should be harmless, but when it's not, it could be misleading.
So, I haven't tried this version 2.0.0 yet, but I wonder what it tags when a PR is merged. Will it tag the resulting commit on the base branch, (the merge_commit_sha described above), or will it tag the last commit on the head branch before the merge?
I've been hacking on a CI build workflow that behaves similarly to what is typically done in TeamCity today:
Comparing the standard TeamCity behavior and the GitHub actions default behavior, the pros and cons are almost mirrors of each other.
This is a snippet of my WIP workflow
jobs:
configure-workflow:
runs-on: ubuntu-latest
outputs:
IS_PRERELEASE: ${{ env.IS_PRERELEASE }}
REF_TO_BUILD: ${{ env.REF_TO_BUILD }}
DO_BUILD: ${{ env.DO_BUILD }}
DO_TAG_RELEASE: ${{ env.DO_TAG_RELEASE }}
DO_NOTIFY_TEAMS: ${{ env.DO_NOTIFY_TEAMS }}
CLEANUP_PRERELEASES: ${{ env.CLEANUP_PRERELEASES }}
steps:
- name: What is the state of this PR?
uses: actions/github-script@v5
with:
# A draft PR should calculate a prerelease version number
# A draft PR should build the "head" branch rather than the trial merged commit
# A draft PR should run all tests with coverage and publish status checks
# A draft PR should NOT tag and upload a GitHub release
# A draft PR whose branch is behind the base should produce a build warning
# A draft PR should NOT advertise its build outcome via Teams
# A ready-to-review PR should calculate a prerelease version number
# A ready-to-review PR should build the "head" branch rather than the trial merged commit
# A ready-to-review PR should run all tests with coverage and publish status checks
# A ready-to-review PR should tag and upload a GitHub release with the built version
# A ready-to-review PR whose branch is behind the base should produce a failed status check
# A ready-to-review PR should advertise its build outcome via Teams
# A closed PR merged _to main_ should calculate a release version number
# A closed PR merged _to main_ should build the merged commit
# A closed PR merged _to main_ should run all tests with coverage and publish status checks
# A closed PR merged _to main_ should tag and upload a GitHub release with the built version
# A closed PR merged _to main_ should advertise its build outcome via Teams
# A closed PR merged _to_main_ should delete prereleases that were previously created from the PR's branch
# A closed PR merged to a branch other than main should NOT build or test anything, and not upload a release.
# A closed PR merged to a branch other than main should delete prereleases that were previously created from the PR's branch
# A closed PR that's not merged should NOT build or test anything, and not upload a release.
# A closed PR that's not merged should delete prereleases that were previously created from the PR's branch
script: |
//NOTE: if this were written in an action, the github.x expressions would need be replaced with different properties on context.
var sourceRef = '${{ github.head_ref }}';
var targetRef = '${{ github.base_ref }}';
var mergeRef = '${{ github.ref }}';
var prIsDraft = '${{ github.event.pull_request.draft }}' === 'true';
var prClosed = '${{ github.event.action }}' === 'closed';
var prMergedToMain = '${{ github.event.pull_request.merged }}' === 'true' && targetRef === 'main';
var isPreRelease = !prMergedToMain
var refToBuildAndTag = sourceRef;
if(prMergedToMain)
refToBuildAndTag = mergeRef;
var doBuild = true;
if(prClosed && !prMergedToMain)
doBuild = false;
var doTagRelease = false;
if(doBuild && !prIsDraft)
{
doTagRelease = true;
}
var doNotifyTeams = doTagRelease;
core.exportVariable('IS_PRERELEASE', isPreRelease);
core.exportVariable('REF_TO_BUILD', refToBuildAndTag);
core.exportVariable('DO_BUILD', doBuild);
core.exportVariable('DO_TAG_RELEASE', doTagRelease);
core.exportVariable('DO_NOTIFY_TEAMS', doNotifyTeams);
core.exportVariable('CLEANUP_PRERELEASES', prClosed);
core.notice(`DO_BUILD=${doBuild}, DO_TAG_RELEASE=${doTagRelease}, REF_TO_BUILD=${refToBuildAndTag}, IS_PRERELEASE=${isPreRelease}, DO_NOTIFY_TEAMS=${doNotifyTeams}, CLEANUP_PRERELEASES=${prClosed}`);
// QUESTION: is the following correct to determine if a PR branch is behind its target?
if(!prClosed) {
// Then we want to warn (or output a status check) if the PR branch is not up-to-date with the base branch
var comparison = await github.rest.repos.compareCommitsWithBasehead({
owner: context.repo.owner,
repo: context.repo.repo,
basehead: `${targetRef}...${sourceRef}`
});
console.log(comparison);
if(prIsDraft) {
core.warning('Branch is behind merge target.');
}
else {
core.error('Branch is behind merge target.');
//TODO: create the failed status check
}
}
dotnet-build-and-test:
runs-on: ubuntu-latest
needs: [configure-workflow]
if: ${{ needs.configure-workflow.outputs.DO_BUILD == 'true' }}
env:
DO_BUILD: ${{ needs.configure-workflow.outputs.DO_BUILD }}
DO_TAG_RELEASE: ${{ needs.configure-workflow.outputs.DO_TAG_RELEASE }}
REF_TO_BUILD: ${{ needs.configure-workflow.outputs.REF_TO_BUILD }}
IS_PRERELEASE: ${{ needs.configure-workflow.outputs.IS_PRERELEASE }}
PROJECT_ROOT: './src'
DEPLOY_ZIP: 'published_app.zip'
REPO_URL: 'https://github.com/${{ github.repository }}'
steps:
- name: Fetch all repo branches
uses: actions/checkout@v2
with:
fetch-depth: 0
ref: ${{ env.REF_TO_BUILD }}
# ... calculate version number, build, test, and optionally package
- name: Create Release with published_artifacts
id: create_release
if: ${{ env.DO_TAG_RELEASE == 'true' }}
uses: im-open/create-release@v1.0.0
with:
github-token: ${{ secrets.GITHUB_TOKEN }} # Special per-job token generated by GH for interacting with the repo
commitish: ${{ env.REF_TO_BUILD }}
delete-existing-release: true # Handy when you hit 're-run jobs' on a workflow run
tag-name: ${{ env.NEXT_VERSION }}
prerelease: ${{ env.IS_PRERELEASE }}
asset-path: ${{ env.PROJECT_ROOT }}/${{ env.DEPLOY_ZIP }}
asset-name: ${{ env.DEPLOY_ZIP }}
asset-content-type: application/zip
#body: # TODO: add release notesSo, I haven't tried this version 2.0.0 yet, but I wonder what it tags when a PR is merged. Will it tag the resulting commit on the base branch, (the
merge_commit_shadescribed above), or will it tag the last commit on the head branch before the merge?
I just saw that you already answered this question in your email:
when a merge to main happens the tag ends up on the last commit from the branch, not the merge commit.
I'm sure that's not what we want. So I believe we will need to conditionally figure out a ref based on the state of the PR so we can supply it as a commitish ref to the im-open/create-release.
The github.sha value doesn't seem to be the sha we want to use for pull_request triggers. It does not match the sha inside of the
github.context.pull_request.head.sha. I still have some confusion about whatgithub.shaactually refers to because it does not match any sha in my log.This PR will use the value from the head branch for pull requests. This will work much better than before where it wasn't tagging the right commit.