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

If DangerID is defined, use it as the context for platform status #840

Merged
merged 1 commit into from
Mar 14, 2019

Conversation

randak
Copy link
Contributor

@randak randak commented Mar 14, 2019

Resolves #839.

Currently, if you run two separate danger builds with their own IDs, they will post two separate comments. However, the status is not tied to the ID, so if one danger build fails and the second one passes, your PR will incorrectly be marked as passing.

This change uses the ID as the context/name for the status, which will allow each build to show up as an independent status, making it possible to have multiple Danger runs on one PR. (We use this to run our build and test in parallel in the CI and report issues from each of them.)


let name = "Danger"
if (process.env["PERIL_INTEGRATION_ID"]) {
name = "Peril"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to support this use case in Peril? I'm not familiar enough with it to know if it's possible to run multiple instances.

Copy link
Member

Choose a reason for hiding this comment

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

Nope, or at least whatever we need to do it'd be a bit of work on Perils side 👍

@randak
Copy link
Contributor Author

randak commented Mar 14, 2019

@orta Can you take a look at the test failure here? It looks like it might not be related to my changes, but if it is let me know

Copy link
Member

@orta orta left a comment

Choose a reason for hiding this comment

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

Yep, cool, looks like the CI issue was just temporary 👍


let name = "Danger"
if (process.env["PERIL_INTEGRATION_ID"]) {
name = "Peril"
Copy link
Member

Choose a reason for hiding this comment

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

Nope, or at least whatever we need to do it'd be a bit of work on Perils side 👍

@orta orta merged commit 227a8d9 into danger:master Mar 14, 2019
@randak randak deleted the support-multiple-statuses branch March 14, 2019 11:28
@randak
Copy link
Contributor Author

randak commented Mar 14, 2019

Awesome, thanks @orta! Any estimate of when this will be published? It'll help us plan whether to go forward with a workaround or wait 😄

@peril-staging
Copy link
Contributor

peril-staging bot commented Mar 14, 2019

Thanks for the PR @randak.

This PR has been shipped in v7.0.15 - CHANGELOG.

@randak
Copy link
Contributor Author

randak commented Mar 14, 2019

Oh, nevermind, Peril answered me, haha 👍

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

Successfully merging this pull request may close these issues.

None yet

2 participants