-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Update getPullRequestsMergedBetween to run pre-set string command #4336
Conversation
@Jag96 from what I see, and I think this what you wanted to achieve, this is logging some extra stuff and not changing the return of the function. But unfortunately due to my lack of experience in the area, I don't know how to test this! Feel free to remove me and find someone else that can give you a better input about this if you need :) |
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.
Definitely worth a shot! Let's test this on the next checklist by CPing it as soon as the checklist is locked.
|
cc @isagoico because I know you'll be more stoked about this than anyone if it works. |
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 ran the test local test and confirmed that it is working. I'll let the merge to you guys because I'm not familiarized enough with the process to test it in staging.
For context, we're holding off on merging this until we have a fresh checklist to confirm the issue is fixed. Putting this on HOLD for now |
Going to CP this to Staging before the prod deploy |
Update getPullRequestsMergedBetween to run pre-set string command (cherry picked from commit 27e9f06)
🚀 Deployed to production in version: 1.0.82-7🚀
|
🚀 Deployed to staging in version: 1.0.82-7🚀
|
🚀 Deployed to staging in version: 1.0.82-8🚀
|
🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀
|
@roryabraham will you please review this
cc @AndrewGable
Details
This PR attempts to fix an issue where when a CP happens, the
toRef
is set to an empty string, even though it is being logged out properly beforehand. Came up with this approach after looking at this SO and seeing that our other usages ofexecSync
only set variables that are separated by spaces, so perhaps there is an issue in this case. I also added a log line that will spit out the command to ensure that the string is what we expect it to be.Fixed Issues
Related to https://github.com/Expensify/Expensify/issues/168349, possibly a fix 🤞
Tests
I ran tests locally to confirm that the function still works as expected:
QA Steps
None
Screenshots
N/A