[build-tools] [ENG-13708] Upload outputs in public step #470
Conversation
Uploading outputs at the end of generic job See: https://linear.app/expo/issue/ENG-13708/move-uploading-outputs-to-eas-build-and-public-step
Added tests, mostly copied from turtle See: https://linear.app/expo/issue/ENG-13708/move-uploading-outputs-to-eas-build-and-public-step
Removed leftover comment See: https://linear.app/expo/issue/ENG-13708/move-uploading-outputs-to-eas-build-and-public-step
radoslawkrzemien
requested review from
szdziedzic and
sjchmiela
as code owners
| } | ||
|
|
||
| try { | ||
| const workflowJobId = nullthrows(ctx.job.builderEnvironment?.env?.__WORKFLOW_JOB_ID); |
There was a problem hiding this comment.
If the user maliciously changes the __WORKFLOW_JOB_ID value to some other workflow ID they won't be able to write outputs values for it because we validate it in WWW, right?
BTW It would be good to move it to job.workflowId so it can't be overridden by the user (not a part of this PR)
| await fetch(new URL(`workflows/${workflowJobId}`, expoApiV2BaseUrl).toString(), { | ||
| method: 'PATCH', | ||
| body: JSON.stringify({ outputs }), | ||
| headers: { | ||
| Authorization: `Bearer ${robotAccessToken}`, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| timeout: 20000, | ||
| }); |
There was a problem hiding this comment.
I believe that fetch won't throw if the status code is for example 500 by default. You need to check it manually. It will only throw on things like timeouts.
| await fetch(new URL(`workflows/${workflowJobId}`, expoApiV2BaseUrl).toString(), { | |
| method: 'PATCH', | |
| body: JSON.stringify({ outputs }), | |
| headers: { | |
| Authorization: `Bearer ${robotAccessToken}`, | |
| 'Content-Type': 'application/json', | |
| }, | |
| timeout: 20000, | |
| }); | |
| const response = await fetch(new URL(`workflows/${workflowJobId}`, expoApiV2BaseUrl).toString(), { | |
| method: 'PATCH', | |
| body: JSON.stringify({ outputs }), | |
| headers: { | |
| Authorization: `Bearer ${robotAccessToken}`, | |
| 'Content-Type': 'application/json', | |
| }, | |
| timeout: 20000, | |
| }); | |
| if (!response.ok) { | |
| throw new Error(...) | |
| } |
| timeout: 20000, | ||
| }); | ||
| } catch (err) { | ||
| logger.error({ err }, 'Failed to upload outputs'); |
There was a problem hiding this comment.
Shouldn't we fail the workflow run altogether when an error is thrown? If you expect outputs from this workflow in your other runs I guess it would be hard to debug later on if we mark this run as a success but some required inputs will be missing.
What we can do is retry outputs upload a couple of times and if it still fails throw an error and mark workflow as errored.
| }); | ||
| logger.info('Uploading outputs'); | ||
|
|
||
| await fetch(new URL(`workflows/${workflowJobId}`, expoApiV2BaseUrl).toString(), { |
There was a problem hiding this comment.
do we want to add retries for it? I believe they would be useful
| }); | ||
| logger.info('Uploading outputs'); | ||
|
|
||
| await fetch(new URL(`workflows/${workflowJobId}`, expoApiV2BaseUrl).toString(), { |
There was a problem hiding this comment.
we can add a util like turtleFetch here if we want to:
https://github.com/expo/turtle-v2/blob/998063fdfb9edb61eba3e080a960a0de067139ab/src/libs/turtle-common/src/turtleFetch.ts#L16-L74
There was a problem hiding this comment.
It used to use turtleFetch when it was located in turtle, but it's unavailable here so I replace it with the standard fetch, but you're right that we need something similar, if only just for retries
| return `${jsepEval(expression, interpolationContext)}`; | ||
| }); | ||
|
|
||
| jobOutputs[outputKey] = outputValue; |
There was a problem hiding this comment.
the jobOutputs can be only of type string right?
Why
https://linear.app/expo/issue/ENG-13708/move-uploading-outputs-to-eas-build-and-public-step
How
Created outputs upload function based on the one existing in turtle, but accepting the
loggerandexpoApiV2BaseUrlparams.Calling that function at the end of generic job in a new dedicated
Complete jobstep, with the logger for that step.Added mappings for that new step.
Test Plan
Automated tests mostly based on those from Turtle
Deployment plan
build-toolsandeas-build-jobeas-build-jobinwebsite, merge and deploybuild-toolsandeas-build-jobin Turtle, merge and deploy