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.

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

[BUG] Commit status check associated with the wrong commit in CircleCI + GitHub combination #1423

Open
valscion opened this issue Feb 1, 2024 · 2 comments
Labels

Comments

@valscion
Copy link
Contributor

valscion commented Feb 1, 2024

Describe the bug

Danger associates the commit status check with the latest commit in an open GitHub pull request when running in CircleCI.

This is problematic when the pull request has more Danger runs and the later Danger run completes faster than the original Danger run. This makes the entire pull request status checks red if Danger has failures.

The commit where Danger did complain on purpose had these environment variables:

Using build environment variables:
  BASH_ENV=/tmp/.bash_env-65bb930c74d4b3238d055ed2-0-build
  CI=true
  CIRCLECI=true
  CIRCLE_BRANCH=depfu/update/rubocop-1.60.2
  CIRCLE_BUILD_NUM=467700
  CIRCLE_BUILD_URL=https://circleci.com/gh/venuu/venuu/467700
  CIRCLE_JOB=checks
  CIRCLE_NODE_INDEX=0
  CIRCLE_NODE_TOTAL=1
  CIRCLE_PROJECT_REPONAME=venuu
  CIRCLE_PROJECT_USERNAME=venuu
  CIRCLE_PULL_REQUEST=https://github.com/venuu/venuu/pull/11423
  CIRCLE_PULL_REQUESTS=https://github.com/venuu/venuu/pull/11423
  [email protected]:venuu/venuu.git
  CIRCLE_SHA1=41ee21fb710533f380322143e0b83eace85215ac
  CIRCLE_SHELL_ENV=/tmp/.bash_env-65bb930c74d4b3238d055ed2-0-build
  CIRCLE_USERNAME=Venuusaur
  CIRCLE_WORKFLOW_ID=ee20da3d-45cb-43da-b145-bcbb9b3627b0
  CIRCLE_WORKFLOW_JOB_ID=d7c3eecb-a9ea-4467-a9f4-d4193408f7c9
  CIRCLE_WORKFLOW_WORKSPACE_ID=ee20da3d-45cb-43da-b145-bcbb9b3627b0
  CIRCLE_WORKING_DIRECTORY=~/venuu
  CI_PULL_REQUEST=https://github.com/venuu/venuu/pull/11423

The commit where Danger didn't complain but which completed sooner than the one that did complain:

Using build environment variables:
  BASH_ENV=/tmp/.bash_env-65bb9358f228fb22b6c5e750-0-build
  CI=true
  CIRCLECI=true
  CIRCLE_BRANCH=depfu/update/rubocop-1.60.2
  CIRCLE_BUILD_NUM=467715
  CIRCLE_BUILD_URL=https://circleci.com/gh/venuu/venuu/467715
  CIRCLE_JOB=checks
  CIRCLE_NODE_INDEX=0
  CIRCLE_NODE_TOTAL=1
  CIRCLE_PROJECT_REPONAME=venuu
  CIRCLE_PROJECT_USERNAME=venuu
  CIRCLE_PULL_REQUEST=https://github.com/venuu/venuu/pull/11423
  CIRCLE_PULL_REQUESTS=https://github.com/venuu/venuu/pull/11423
  [email protected]:venuu/venuu.git
  CIRCLE_SHA1=c995a322ff042af4574820728638a5450eed8cbf
  CIRCLE_SHELL_ENV=/tmp/.bash_env-65bb9358f228fb22b6c5e750-0-build
  CIRCLE_USERNAME=valscion
  CIRCLE_WORKFLOW_ID=9c2dd38a-d5d7-4e91-bf20-be114c9134aa
  CIRCLE_WORKFLOW_JOB_ID=6de8b27a-33a4-45a8-8d5e-25e6541e6a2e
  CIRCLE_WORKFLOW_WORKSPACE_ID=9c2dd38a-d5d7-4e91-bf20-be114c9134aa
  CIRCLE_WORKING_DIRECTORY=~/venuu
  CI_PULL_REQUEST=https://github.com/venuu/venuu/pull/11423

To Reproduce

Steps to reproduce the behavior:

  1. Setup CircleCI to run Danger
  2. Create two commits: One that has a Danger failure but is slow to run and one that doesn't fail and is quick to run
  3. Create a GitHub PR with only the slow and failing commit pushed first
  4. Push the second commit immediately once the first commit starts to run in CI
  5. See that the second commit gets the failing status check even though the first commit was failing

Expected behavior

Danger should associate the GitHub commit status with the commit that was failing and not with the latest commit in the PR.

If possible, Danger should use the CIRCLE_SHA1 environment variable to associate the status check with the correct commit.

Screenshots

image

image

Your Environment

software version
danger.js 11.3.1
node
npm
Operating System

Additional context

N/A

@valscion valscion added the bug label Feb 1, 2024
@valscion
Copy link
Contributor Author

valscion commented Feb 1, 2024

I wonder if the call in here is missing the last optional argument depicting what the commit hash is?

const successPosting = await this.platform.updateStatus(passed, messageForResults(results), urlForInfo, dangerID)

Looking at what this code could use:

updateStatus = async (
passed: boolean | "pending",
message: string,
url?: string,
dangerID?: string,
ciCommitHash?: string
): Promise<any> => {

EDIT: Looking at the codepath this seems to be correct for us. We are not using inline comments but are instead in favor of posting a single comment that Danger keeps up-to-date and using the commit status checks here.

The fix looks fairly trivial. I can try doing one if you so prefer.

@fbartho
Copy link
Member

fbartho commented Apr 11, 2024

@valscion We’re always happy to review PRs fixing issues. This is a collaborative open-source project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants