-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Ingest Manager] Support both zip & tar archives from Registry #76197
Changes from 2 commits
a3c303f
e5c0e92
c230acc
9c282d1
547a8e0
50bc169
e4fd596
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,8 +17,8 @@ import { | |
RegistrySearchResults, | ||
RegistrySearchResult, | ||
} from '../../../types'; | ||
import { cacheGet, cacheSet, getCacheKey, cacheHas } from './cache'; | ||
import { ArchiveEntry, untarBuffer } from './extract'; | ||
import { cacheGet, cacheSet, cacheHas, getArchiveKey, setArchiveKey } from './cache'; | ||
import { ArchiveEntry, untarBuffer, unzipBuffer } from './extract'; | ||
import { fetchUrl, getResponse, getResponseStream } from './requests'; | ||
import { streamToBuffer } from './streams'; | ||
import { getRegistryUrl } from './registry_url'; | ||
|
@@ -182,17 +182,20 @@ async function extract( | |
onEntry: (entry: ArchiveEntry) => void | ||
) { | ||
const archiveBuffer = await getOrFetchArchiveBuffer(pkgName, pkgVersion); | ||
const archiveKey = getArchiveKey(pkgName, pkgVersion); | ||
// shouldn't be possible since getOrFetchArchiveBuffer -> fetchArchiveBuffer sets the key | ||
if (!archiveKey) throw new Error('no archive key'); | ||
const isZip = archiveKey.endsWith('.zip'); | ||
const libExtract = isZip ? unzipBuffer : untarBuffer; | ||
|
||
return untarBuffer(archiveBuffer, filter, onEntry); | ||
return libExtract(archiveBuffer, filter, onEntry); | ||
} | ||
|
||
async function getOrFetchArchiveBuffer(pkgName: string, pkgVersion: string): Promise<Buffer> { | ||
// assume .tar.gz for now. add support for .zip if/when we need it | ||
const key = getCacheKey(`${pkgName}-${pkgVersion}`); | ||
let buffer = cacheGet(key); | ||
const key = getArchiveKey(pkgName, pkgVersion); | ||
let buffer = key && cacheGet(key); | ||
if (!buffer) { | ||
buffer = await fetchArchiveBuffer(pkgName, pkgVersion); | ||
cacheSet(key, buffer); | ||
} | ||
|
||
if (buffer) { | ||
|
@@ -203,16 +206,21 @@ async function getOrFetchArchiveBuffer(pkgName: string, pkgVersion: string): Pro | |
} | ||
|
||
export async function ensureCachedArchiveInfo(name: string, version: string) { | ||
const pkgkey = getCacheKey(`${name}-${version}`); | ||
if (!cacheHas(pkgkey)) { | ||
const pkgkey = getArchiveKey(name, version); | ||
if (!pkgkey || !cacheHas(pkgkey)) { | ||
await getArchiveInfo(name, version); | ||
} | ||
} | ||
|
||
async function fetchArchiveBuffer(pkgName: string, pkgVersion: string): Promise<Buffer> { | ||
const { download: archivePath } = await fetchInfo(pkgName, pkgVersion); | ||
const registryUrl = getRegistryUrl(); | ||
return getResponseStream(`${registryUrl}${archivePath}`).then(streamToBuffer); | ||
const archiveUrl = `${getRegistryUrl()}${archivePath}`; | ||
const buffer = await getResponseStream(archiveUrl).then(streamToBuffer); | ||
|
||
setArchiveKey(pkgName, pkgVersion, archivePath); | ||
cacheSet(archivePath, buffer); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ensure we always cache a fetched archive buffer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this cache invalidated if the current package is removed? This is mainly important for package development. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ruflin No, but that's the same behavior as now. I'm not sure if this means it is an issue or if it's being avoided/worked around in some other way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @skh @neptunian I thought one of you solve this issue recently or at least debugged it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is #68890 -- it's still open because I misunderstood the original description and tested the wrong things, and then it got moved back in the queue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
return buffer; | ||
} | ||
|
||
export function getAsset(key: string) { | ||
|
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.
moved the caching from this
getOrFetch
function to inside thefetchArchiveBuffer