DOP-4288: Add correct error handling to downloadBuildDependencies #979
Conversation
|
Your feature branch infrastructure has been deployed! Your webhook URL is: https://ak2xarzkwl.execute-api.us-east-2.amazonaws.com/prod/webhook/githubEndpoint/trigger/build For more information on how to use this endpoint, follow these instructions. |
There was a problem hiding this comment.
Looks good. I'd love more input from others on if we want an error in curling dependencies to fail the build. I lean toward yes. If a dependency does not download, I think we maybe want to have that be a fatal error. Maybe that's another ticket? Or maybe that's part of this one.
| }); | ||
|
|
||
| const responseSync = await Promise.all(response); | ||
| commands = commands.concat(responseSync); |
There was a problem hiding this comment.
Such a nit: I think I'd rather see us keep commands as a constant, and use the push method rather than concat which rewrites the entire array each time.
I would agree with this. An error in a build dependency would most likely cause downstream errors in built pages anyways, so I imagine failing the build earlier would be beneficial. I think it would make sense to make this part of the same ticket if it's simple to do and test, otherwise a separate ticket also sgtm |
|
@rayangler @mmeigs what do you guys think about failing on the first dependency (example log here, easy enough to implement)? This, however, doesn't show the status of successful downloads or other downloads that might fail in the future. I think that would also be pretty easy to implement, but I'm curious if you guys have any thoughts on what the build failure would ideally look like. |
|
I think I'm okay with either of the two approaches. Showing all failed dependencies together instead of just a single one might be the best in terms of convenience, so I think this would be my preference between the two if it's simple enough to do |
| .get(dep.url, { timeout: 10000, responseType: 'stream' }) | ||
| .then((res) => { | ||
| res.data.pipe(fs.createWriteStream(`${rootDir}${targetDir}/${dep.filename}`)); | ||
| return `Downloading ${dep.url} into ${rootDir}${targetDir}/${dep.filename}`; |
There was a problem hiding this comment.
I noticed that we see many of the dependency files that are not present when downloading. I think it could be possibly due to the fact that we return before the write stream fully completes, but I will have to do a bit more digging to confirm this.
| const rootDir = targetDir != repoDir ? `${repoDir}/` : ''; | ||
| const curlString = axios | ||
| .get(dep.url, { timeout: 10000, responseType: 'stream' }) | ||
| .then((res) => { |
There was a problem hiding this comment.
NIT: For consistency, it would be nice to use the async/await syntax as opposed to the .then approach for promises if possible.
| dependencyInfo | ||
|
|
||
| const response = dependencyInfo.dependencies.map((dep) => { | ||
| const rootDir = targetDir != repoDir ? `${repoDir}/` : ''; |
There was a problem hiding this comment.
I think we may want to use the full path instead of the relative path for the rootDir. Also, I don't believe targetDir will equal repoDir as repoDir has the current working directory path appended to it.
DOP-4288
This PR corrects
downloadBuildDependenciesby enabling correct logging of successful/unsuccessful downloading of build dependencies. It also switches from usingcurlcommands toaxiosto make the dependency request. Additionally, the build will fail if a dependency is not successfully downloaded.Example build log link here.
To test, you can edit the
optionalBuildSteps.buildDependenciesobject in areposBranchesentry for whatever docs property you want to test with, and mess with the links needed for building the property. Currently I've only really tested 404 errors - would be curious how other errors perform.