-
Notifications
You must be signed in to change notification settings - Fork 13
Return group errors #110
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
Return group errors #110
Conversation
This reverts commit dd74c5f.
src/main.ts
Outdated
console.log("Extracting commit SHA for repro steps"); | ||
const commit = (await execAsync(summary.repoDir, `git rev-parse @`)).trim(); | ||
text += `<li>In dir <code>${summary.repo.name}</code>, run <code>git reset --hard ${commit}</code></li>\n`; | ||
text += `<li>In dir <code>${summary.repo.name}</code>, run <code>git reset --hard ${summary.commit}</code></li>\n`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will end up being a behavior change; this try/catch was for the exec but now it doesn't do anything here, but can do something in the place it was moved to. I would try catch over in the new place to preserve this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct. Fixed. Instead, I now have the try-catch where the commit is extracted and also assigned a nullable value in case we don't get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good for a re-land? Not sure we can test it before running it on another PR.
We can only test it if it's on a branch on the repo itself. |
Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
Returns #105
Fixes the issue caused by this same PR as well. The bug was caused by
tmpfs
being unmounted before using the files for reporting. Now, we read all of the necessary files before unmounting.I appreciate if I can get a review on the latest commit on this PR. (The one that introduces the fix).