-
Notifications
You must be signed in to change notification settings - Fork 13
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
Feature/import release documents from zip file #1692
base: feature/BAI-1252-make-s3-release-zip-files-viewable-in-bailo
Are you sure you want to change the base?
Feature/import release documents from zip file #1692
Conversation
…bailo' into feature/import-release-documents-from-zip-file
}, | ||
} | ||
const result = await importDocuments(res, mirroredModelId, sourceModelId, payloadUrl) | ||
return result |
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.
Setting and then instantly returning a value seems unnecessary - could these two lines be combined?
async function importModelFile(content: Response, importedPath: string, mirroredModelId: string, sourceModelId) { | ||
async function importDocuments(res: Response, mirroredModelId: string, sourceModelId: string, payloadUrl: string) { | ||
const modelCards: ModelCardRevisionInterface[] = [] | ||
// TODO - Use this releases array for auditing purposes |
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.
Yep, I think it would probably make sense to align the release info with the model card info and return the all the semvers that have been imported and the new releases?
} else if (/^releases\/(.*)\.json$/.test(key)) { | ||
releaseJsonStrings.push(Buffer.from(zipContent[key]).toString('utf8')) | ||
} else if (/^files\/(.*)\.json$/.test(key)) { | ||
// TODO - Handle file parsing |
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.
To be done as part of the next subtask
} else if (/^files\/(.*)\.json$/.test(key)) { | ||
// TODO - Handle file parsing | ||
} else { | ||
// TODO - Is this how we want to handle unrecognised zip content? |
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 think so? Do you have an alternative in mind?
) | ||
|
||
releaseJsonStrings.forEach((releaseJson) => { | ||
// TODO - As this is a forEach loop it will not wait for this iteration to finish before handling the next. Check we are okay with this. |
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 don't think we get any advantage here in using a forEach
loop so maybe we should just go with the simpler for loop?
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.
As parseRelease
is no longer async this doesn't matter anymore
No description provided.