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] Unable to run concurrent Danger instances on a single machine. #1311
Comments
For some additional context, here are some snippets from our pipelines: This one failed:
This one worked fine, then presumably cleaned up the file, which broke the other:
Notice theres a URL which is identical in both runs: |
Seems reasonable to me to have the danger-dsl JSON file name be randomized then I guess, I think it was probably only made consistent so that it was easy to look in and verify |
Would a good compromise be something like:
Doing some research last night, it seems we’d have to add a new dependency to generate a UUID, which I’m not thrilled about. Is there another approach to get a unique enough string without external deps? |
Yeah, the crypto module can get you there: import {randomBytes} from "crypto"
const randomString1 = randomBytes(4).toString('hex'); |
Amazing. I’ll do a PR this weekend. |
Describe the bug
We run Danger via Danger Swift, on a self-hosted Mac Mini using GitLab Runners. We have a single runner setup, with
concurrent
enabled.Danger Swift passes in the DSL via URL, which DangerJS writes to the temporary directory:
However, this is problematic as this results in a URL like the following, which both processes try to read/write from at the same time:
Instead we should probably suffix a unique directory, for example:
To Reproduce
Steps to reproduce the behavior:
Expected behavior
The temporary DSL file should be placed in a unique directory, so that one process wont cleanup the DSL while another is using it.
Your Environment
Additional context
I submitted a similar fix for danger-swift, where we use a UUID for a folder name. This works well in my testing. If you're happy with this approach, I'm happy to try create a PR for this change. I'm unaware if there are any complications for doing this though. Checking the source code there are two places where
tmpdir()
is used, and this issue could potentially occur:The disadvantage to this approach is we're then responsible for cleaning up this working directory once the process has finished.
The text was updated successfully, but these errors were encountered: