-
Notifications
You must be signed in to change notification settings - Fork 109
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
[WIP] GitHub code: tar processing #3050
Conversation
Left to do is to create directories objects so that it's super easy to use from the activity that will call into it. |
8530ad6
to
0edaa90
Compare
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.
LGTM, left a couple of comments.
{ | ||
owner: login, | ||
repo: repoName, | ||
ref: defaultBranch, |
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.
Reading the code one can provide "parseSuccessResponseBody": false
to have a stream. See https://github.com/octokit/request.js?tab=readme-ov-file#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.
Example: octokit/octokit.js#2369
} | ||
|
||
// Save the tarball to the temp directory. | ||
await asyncPipeline(tarballStream as Readable, createWriteStream(tarPath)); |
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.
nit
pipeline are useful when there is an operation in between read and write. Here a simple pipe
could do the trick:
Something like:
const fileStream = createWriteStream(tarPath);
request(<gh_link>).pipe(fileStream);
for await (const file of getFiles(tempDir)) { | ||
console.log(file); | ||
// get file extension | ||
const ext = file.split(".").pop(); |
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.
nit
const ext = file.split(".").pop(); | |
const ext = path.extname(file); |
EXTENSION_WHITELIST.includes(`.${ext}`) || | ||
FILENAME_WHITELIST.includes(file); | ||
|
||
const isUnderLimite = size < 1024 * 1024; |
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.
const isUnderLimite = size < 1024 * 1024; | |
const isUnderLimit = size < 1024 * 1024; |
.toString("hex") | ||
.substring(0, 16)}`; | ||
|
||
const fileName = file.split("/").pop() || ""; |
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.
You could abstract all of this by using the native path
library.
// This function returns file and directories object with parent, internalIds, and sourceUrl | ||
// information. The root of the directory is considered the null parent (and will have to be | ||
// stitched by the activity). | ||
export async function processRepository( |
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.
nit
maybe split this function a bit.
`Files total size: ${files.reduce((acc, f) => acc + f.sizeBytes, 0)}` | ||
); | ||
|
||
await cleanUpProcessRepository(tempDir); |
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.
Should we wrap the code above with a try/catch/finally
so we always remove it afterward?
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.
This is just for testing and the only thing that can really fail is processRepository, but if it fails we don't know the tempDir :/
I'll add a clean-up in a try/catch/finally in processRepository
Fixes #3030
Function to:
The cli will let me check what our repo looks like