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

Allow parsing remote files as typescript #1366

Merged
merged 4 commits into from
Mar 2, 2023

Conversation

snowe2010
Copy link
Contributor

@snowe2010 snowe2010 commented Mar 2, 2023

Currently, the transpiler.ts makes an assumption that the filename will always end with .ts, but this is incorrect in the case of remote dangerfiles that have refs attached, for example abc/123/dangerfile.ts?feature/branch
Due to this, typescript parsing will always fail for remote files.

This commit adds a new parameter to the transpiler function, which parses the filename differently if the file is a remote one. This change also had to cascade up the stack to the runDangerfileEnvironment function, where we now accept a set of tuples, where the first is the filename, and the second is if it is remote or not. The only place the remote flag is being set currently is in the GitHub runner, where we download the remote dangerfile.

Finally, the pr subcommand failed to use the remote feature correctly, since the file would never exist when the command is first run. The check for the file has been removed, simply leaving behind the same check that is everywhere else.

Fixes: #1359

snowe2010 and others added 4 commits March 1, 2023 21:51
Currently, the transpiler.ts makes an assumption that the filename will
always end with .ts, but this is incorrect in the case of remote
dangerfiles that have refs attached, for example
abc/123/dangerfile.ts?feature/branch
Due to this, typescript parsing will always fail for remote files.

This commit adds a new parameter to the transpiler function, which
parses the filename differently if the file is a remote one. This change
also had to cascade up the stack to the runDangerfileEnvironment
function, where we now accept a set of tuples, where the first is the
filename, and the second is if it is remote or not. The only place the
remote flag is being set currently is in the GitHub runner, where we
download the remote dangerfile.

Finally, the pr subcommand failed to use the remote feature correctly,
since the file would never exist when the command is first run. The
check for the file has been removed, simply leaving behind the same
check that is everywhere else.
@orta orta enabled auto-merge March 2, 2023 09:28
@orta
Copy link
Member

orta commented Mar 2, 2023

This makes sense WRT #1359 - when we used remote dangerfiles in Artsy it always was though Peril's runtime and so this flow largely would have been quick verifications from me for others. Great spot!

@orta orta merged commit 89fabea into danger:main Mar 2, 2023
@orta
Copy link
Member

orta commented Mar 2, 2023

Thanks, this is shipped as 11.2.4!

@snowe2010
Copy link
Contributor Author

Wow, I didn't expect a merge first try haha! Thank you very much!

@snowe2010 snowe2010 deleted the feature/fix-remote-dangerfile-parsing branch March 2, 2023 17:26
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.

2 participants