Feature/azdo requester #7
Conversation
Add model for the DB Add validation of initial request Simplify the logs fetching Change slug to reflect the dual Build/Release nature of jobs in Azure DevOps (i.e. user can invoke npm publish both from a Build and a Release job agent's console) Clean the (revoked) token and run from the branch
Node lib is quite large and also inconsistent with the rest of the requesters
Add the azure config model and table Remove reference to the azdo-node-api
Finish first draft on the Requester Fix a comment in Base Requester
|
@MarshallOfSound what say you, sir? |
| return attemptValidateProof(attempts - 1); | ||
| }; | ||
|
|
||
| return getLogs(config, request.requestMetadata.releaseId) |
There was a problem hiding this comment.
Can this be converted to use async/await and try {} catch {} instead
There was a problem hiding this comment.
Sure, we can.
Do we want to do that for consistency reasons? Or something else?
There was a problem hiding this comment.
Partly consistency but mostly to ensure we maintain the promise chain for things like top level error handling. Using .then in some places can cause the chain to become detached meaning if the promise rejects it will be an unhandled rejection. Using async await everywhere maintains the chain much easier
There was a problem hiding this comment.
Using .then in some places can cause the chain to become detached meaning if the promise rejects it will be an unhandled rejection.
Hm, I did not know that. Looking into it. Did you know it a resource I can use to learn about that?
| * config.accessToken = 'easasdasdasdasdasda12d2312d13ed1d'; | ||
| * releaseId === 58 | ||
| */ | ||
| export function getLogs( |
There was a problem hiding this comment.
Can we do this entirely in memory instead of writing to disk, I've seen zip-based file system attacks before and I don't want to be susceptible to an out-of-zip write attack or something.
There are a few nodejs zip libraries you could probably use to do the work in-memory
There was a problem hiding this comment.
Potentially just using the fromBuffer method of this zip library you're currently using.
https://www.npmjs.com/package/yauzl#frombufferbuffer-options-callback
There was a problem hiding this comment.
Sure, especially if there is a potential security vulnerability.
I wanted to keep memory consumption low by not keeping the buffer/zip stream and string at the same time.
There was a problem hiding this comment.
I think paying the in-memory price here is ok, these log zips shouldnt be too big and theyll only be in memory for a second or two.
Yes - I did no want to push a lot of changed files at once. Next in line would be the UI for adding a AzDO requester to a project. And the server endpoint for it too (not looked into that one yet) |
Lots of prettier changes...
|
@MarshallOfSound how does it seem now? |
|
About next step - the UI and endpoint. We can merge this PR and start a new PR . |
|
@MarshallOfSound what do you think - is this ready to merge? With the changes. |
As well as some failure conditions
|
@MarshallOfSound kindly reminding :) |
|
Hey @gparlakov sorry for the delay here, lot of stuff on my plate at the moment 😄
Can you do this, a sequence of smaller PRs that I can land at the same time would probably be best |
|
@MarshallOfSound opened a PR gparlakov#2 |
|
@MarshallOfSound did you have a chance to look at the PR opened here? |
Add requester, db model and migration.
Added package https://bundlephobia.com/result?p=yauzl@2.10.0