-
Notifications
You must be signed in to change notification settings - Fork 7
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
actions: promote release action #142
Conversation
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
8e7f6cd
to
5d94b1c
Compare
scripts/promote-release.js
Outdated
let r2Error; | ||
|
||
for (let i = 0; i < RETRY_LIMIT; i++) { | ||
try { |
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.
Use Promise alike then.catch
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.
Like
for (let i = 0; i < R2_RETRY_COUNT; i++) {
result
.then(/*...*/)
.catch(/*...*/)
}
?
If so it would send the same request multiple times regardless if it succeeded or not since there's nothing blocking the execution context (like await
is doing here)
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 see you want to await on it. You can still do:
const somethingThatReturnsPromise = invokeSomething.catch();
await somethingThatReturnsPromise;
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.
Worth noting this is just a copy of this function here
release-cloudflare-worker/src/utils/provider.ts
Lines 8 to 28 in 09a0891
export async function retryWrapper<T>( | |
request: () => Promise<T>, | |
retryLimit: number, | |
sentry?: Toucan | |
): Promise<T> { | |
let r2Error: unknown = undefined; | |
for (let i = 0; i < retryLimit; i++) { | |
try { | |
const result = await request(); | |
return result; | |
} catch (err) { | |
r2Error = err; | |
} | |
} | |
if (sentry !== undefined) { | |
sentry.captureException(r2Error); | |
} | |
throw r2Error; | |
} |
Why .catch()
over the try/catch?
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 left a few nits; I also added screenshots of it running and working on a dry run.
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]> Signed-off-by: flakey5 <[email protected]>
75a68db
to
fcd95a9
Compare
|
||
for (let i = 0; i < R2_RETRY_COUNT; i++) { | ||
try { | ||
const result = await request(); |
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.
In general await with a return is not needed? As an await
on parent function call is also needed and enough.
const result = await request(); | ||
return result; | ||
} catch (err) { | ||
r2Error = err; |
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.
Shouldn't the throw happen here already?
const recursive = | ||
process.argv.length === 4 && process.argv[3] === '--recursive'; | ||
|
||
if (recursive) { |
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.
Instead of if else, when not recursive make an array of one file :)
Follow up pr will be made addressing comments |
Re nodejs/build#3838 (comment)