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

Suffix temporary directory with unique path #530

Merged
merged 3 commits into from
Aug 5, 2022
Merged

Suffix temporary directory with unique path #530

merged 3 commits into from
Aug 5, 2022

Conversation

squarefrog
Copy link
Contributor

Fixes #529

Reusing the temporary directory can cause issues with concurrent instances of Danger. By appending a unique identifier we can prevent race conditions when running Danger from concurrent processes.

Copy link
Member

@f-meloni f-meloni left a comment

Choose a reason for hiding this comment

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

Thank you! Can you please update the CHANGELOG file as well? :)

@squarefrog
Copy link
Contributor Author

Of course! Looks like I've broken all of the pipelines though :(

Fixes #529

Reusing the temporary directory can cause issues with concurrent
instances of Danger. By appending a unique identifier we can prevent
race conditions when running Danger from concurrent processes.
@squarefrog
Copy link
Contributor Author

All set for review now @f-meloni, sorry for the CI spam!

When the runner is run, it creates a new temporary directory with a unique ID, for example:

/var/folders/86/1vtftp_j2rn_wdcq6232q_z80000gq/T/danger/4FE1BABB-D918-4FB6-9CCF-D27B45034149/

The temporary files are then created within this folder. Upon completion (either success or failure), then the unique folder is cleaned up leaving only the parent folder:

/var/folders/86/1vtftp_j2rn_wdcq6232q_z80000gq/T/danger/

One thing I'm unsure of though, is the dslJSONPath. I'm unsure where this is as it's passed from Danger JS, so I've left the cleanup for that as-is.

@squarefrog
Copy link
Contributor Author

Do I need to do anything else to get this merged in?

@f-meloni
Copy link
Member

f-meloni commented Aug 5, 2022

No, I thought I merged it already, sorry

@f-meloni f-meloni merged commit 263436d into danger:master Aug 5, 2022
@squarefrog
Copy link
Contributor Author

Thanks so much!

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.

Danger causing unexpected pipeline failures with concurrency enabled
2 participants