Create retarget-open-prs utility
#45230
Conversation
sarayourfriend
added
the
[Status] Needs Review
sarayourfriend
changed the title
retarget-open-prs utility
sarayourfriend
force-pushed
the
add/retarget-open-prs
branch
from
c62af0b
to
c203bfb
Compare
|
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Nice work with this one @saramarcondes!
I'll let @scinos to provide his feedback, but I'm leaving some thoughts and questions, none of which are really blocking IMHO. It mostly depends on how much we want to focus on making this one error-proof from the first iteration 😉
packages/retarget-open-prs/index.js
Outdated
| } ); | ||
|
|
||
| if ( remainingRateLimit === 0 ) { | ||
| const rateLimitResetDate = new Date( rateLimitReset * 1000 ); |
There was a problem hiding this comment.
Maybe we could just make the script wait for that time instead? That way we won't have to execute it again, but we can just wait for it to restart automatically once rate limit is reset.
There was a problem hiding this comment.
The amount of time I'm observing locally can be up to an hour, which seems like an awful long time for a script to stall. Do you think we should determine a tolerance? Maybe if the time is within the next ~minute or so we wait but otherwise escape with the message?
There was a problem hiding this comment.
That's the thing, maybe I want the script to run in the background while I'm working on other things 😉
We could have a config flag for that, and keep the default behavior as you described. What do you think?
There was a problem hiding this comment.
That sounds good to me!
| describe: | ||
| 'The original branch name against which to select open PRs that need to be retargted.', | ||
| demandOption: true, | ||
| requiresArg: true, |
There was a problem hiding this comment.
Do you think it makes sense to always default to the default branch here? We could fetch it from the default_branch field at https://docs.github.com/en/rest/reference/repos#get-a-repository
There was a problem hiding this comment.
What are your thoughts on this @saramarcondes? Perhaps this suggestion would be over-engineering, but I was just curious to know what you think about it.
There was a problem hiding this comment.
Yeah, I thought about doing this but it didn't feel necessary for something that could be too easily unintentional depending on the use case. It doesn't save but 10 keystrokes in most cases.
sarayourfriend
force-pushed
the
add/retarget-open-prs
branch
from
1f62dfa
to
4c4e417
Compare
sarayourfriend
force-pushed
the
add/retarget-open-prs
branch
from
59a91c6
to
6df6c18
Compare
There was a problem hiding this comment.
Code is looking great to me 👍 Nice work with this @saramarcondes! I don't see anything preventing this PR from shipping at this point.
@scinos did you have any further thoughts on this PR? I'd love to get your expertised mind to provide some feedback, if possible 😉
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
matticbot
removed
the
[Status] Needs Review
Changes proposed in this Pull Request
retarget-open-prsthat retargets open pull requests in a repository from one branch to anotherTesting instructions
Related to #43395