-
Notifications
You must be signed in to change notification settings - Fork 1
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
[Feat] Add folder uploading #33
Changes from 7 commits
faa49c0
c0ddd3a
7619f2e
e9c6bd4
e8855cb
f4b4ab4
4794124
ff05d68
7285b11
26ebb3a
33d503c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,20 @@ | ||
/* eslint-disable @typescript-eslint/no-unused-vars */ | ||
import path from "path"; | ||
import fs from "fs"; | ||
import mime from "mime-types"; | ||
import { CliUx } from '@oclif/core'; | ||
import { ReleaseMeta } from "@valist/sdk"; | ||
import { SupportedPlatform } from '@valist/sdk/dist/typesShared'; | ||
import { zipDirectory } from './zip'; | ||
import { ReleaseConfig } from './types'; | ||
import { getZipName } from './utils/getZipName'; | ||
import { DesktopPlatform, WebPlatform, getSignedUploadUrls, uploadFileS3 } from '@valist/sdk/dist/s3'; | ||
import fs from "fs"; | ||
import { getSignedUploadUrls, uploadFileS3 } from '@valist/sdk/dist/s3'; | ||
import { AxiosInstance } from 'axios'; | ||
|
||
interface PlatformEntry { | ||
platform: string | ||
path: string | ||
installScript: string | ||
executable: string | ||
platform: string; | ||
path: string; | ||
installScript: string; | ||
executable: string; | ||
} | ||
|
||
const baseGateWayURL = `https://gateway-b3.valist.io`; | ||
|
@@ -22,11 +23,13 @@ export async function uploadRelease(client: AxiosInstance, config: ReleaseConfig | |
const updatedPlatformEntries: PlatformEntry[] = await Promise.all(Object.entries(config.platforms).map(async ([platform, platformConfig]) => { | ||
const installScript = platformConfig.installScript; | ||
const executable = platformConfig.executable; | ||
|
||
if (config && config.platforms[platform] && !config.platforms[platform].zip) { | ||
return { platform, path: platformConfig.path, installScript, executable } | ||
return { platform, path: platformConfig.path, installScript, executable }; | ||
} | ||
|
||
const zipPath = getZipName(platformConfig.path); | ||
CliUx.ux.action.start(`zipping ${zipPath}`); | ||
CliUx.ux.action.start(`Zipping ${zipPath}`); | ||
await zipDirectory(platformConfig.path, zipPath); | ||
CliUx.ux.action.stop(); | ||
return { platform, path: zipPath, installScript, executable }; | ||
|
@@ -41,78 +44,109 @@ export async function uploadRelease(client: AxiosInstance, config: ReleaseConfig | |
external_url: `${baseGateWayURL}/${releasePath}`, | ||
platforms: {}, | ||
}; | ||
CliUx.ux.action.start('uploading files'); | ||
|
||
const platformsToSign: Partial<Record<SupportedPlatform, DesktopPlatform | WebPlatform>> = {}; | ||
CliUx.ux.action.start('Uploading files'); | ||
|
||
for (const platformEntry of updatedPlatformEntries) { | ||
const platformKey = platformEntry.platform as SupportedPlatform; | ||
const { path, executable } = platformEntry; | ||
const file = fs.createReadStream(path); | ||
const { path: platformPath, executable } = platformEntry; | ||
|
||
// Handle WebGL folder upload, otherwise treat as a single file | ||
const isWebGL = platformKey === 'webgl'; | ||
const files = isWebGL ? await getFolderFiles(platformPath) : await getSingleFile(platformPath); | ||
|
||
Comment on lines
+69
to
+70
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. We should try to avoid adding if statements throughout the code for various platform key specific scenarios. It keeps us from making more general interfaces and the code is harder to read/more complicated Could we add a field on the config hyperplay.yml file for Then we would get its value here like
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. I generally agree, though I still think it's a balance between clean code and developer experience, realistically we only are going to support a very small set of platforms for the foreseeable future, I think it's way more worth it to provide as minimal a configuration and as good of a developer experience before purity of interfaces, or future state code complexity. I think isFolderUpload is a great idea but if the intended purpose of the webgl builds is for it to upload a folder of webgl assets it seems like it only creates more friction to make the publisher configure additional fields. Either way it's a good to have config, just wanted to express some thought's around dev ux vs code purity. |
||
platformsToSign[platformKey] = { | ||
CliUx.ux.action.start(`Generating presigned URLs for ${platformKey}`); | ||
const urls = await getSignedUploadUrls({ | ||
account: config.account, | ||
project: config.project, | ||
release: config.release, | ||
platform: platformKey, | ||
files: file, | ||
executable, | ||
}; | ||
files: files.map(file => ({ | ||
fileName: file.fileName, | ||
fileType: mime.lookup(file.filePath) || 'application/octet-stream', | ||
fileSize: file.fileSize, | ||
})), | ||
type: "release", | ||
}, { client }); | ||
CliUx.ux.action.stop(); | ||
|
||
for (const url of urls) { | ||
const fileData = isWebGL ? files.find(f => f.fileName === url.fileName)?.filePath : platformPath; | ||
if (!fileData) throw new Error(`File data not found for ${url.fileName}`); | ||
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. we could replace |
||
|
||
const fileType = mime.lookup(fileData) || 'application/octet-stream'; | ||
const progressIterator = uploadFileS3( | ||
fs.createReadStream(fileData), | ||
url.uploadId, | ||
url.key, | ||
url.partUrls, | ||
fileType, | ||
{ client } | ||
); | ||
|
||
let location = ''; | ||
for await (const progressUpdate of progressIterator) { | ||
if (typeof progressUpdate === 'number') { | ||
CliUx.ux.log(`Upload progress for ${platformKey} - ${url.fileName}: ${progressUpdate}%`); | ||
} else { | ||
location = progressUpdate; | ||
} | ||
} | ||
|
||
if (!location) throw new Error('No location returned after upload'); | ||
|
||
const fileStat = await fs.promises.stat(fileData); | ||
const downloadSize = fileStat.size.toString(); | ||
|
||
// Add platform metadata after successful upload | ||
meta.platforms[platformKey] = { | ||
executable, | ||
name: url.fileName, | ||
external_url: `${baseGateWayURL}${location}`, | ||
downloadSize, | ||
installSize: downloadSize, | ||
installScript: platformEntry.installScript, | ||
}; | ||
} | ||
} | ||
|
||
CliUx.ux.action.start("Generating presigned urls"); | ||
const urls = await getSignedUploadUrls( | ||
config.account, | ||
config.project, | ||
config.release, | ||
platformsToSign, | ||
{ | ||
client, | ||
}, | ||
); | ||
CliUx.ux.action.stop(); | ||
return meta; | ||
} | ||
|
||
const signedPlatformEntries = Object.entries(platformsToSign); | ||
for (const [name, platform] of signedPlatformEntries) { | ||
const preSignedUrl = urls.find((data) => data.platformKey === name); | ||
if (!preSignedUrl) throw "no pre-signed url found for platform"; | ||
|
||
const { uploadId, partUrls, key } = preSignedUrl; | ||
const fileData = platform.files as fs.ReadStream; | ||
|
||
let location: string = ''; | ||
const progressIterator = uploadFileS3( | ||
fileData, | ||
uploadId, | ||
key, | ||
partUrls, | ||
{ | ||
client, | ||
} | ||
); | ||
// Helper function to gather all files in a folder for WebGL uploads | ||
async function getFolderFiles(folderPath: string): Promise<Array<{ fileName: string, filePath: string, fileSize: number }>> { | ||
const fileList: Array<{ fileName: string, filePath: string, fileSize: number }> = []; | ||
|
||
for await (const progressUpdate of progressIterator) { | ||
if (typeof progressUpdate === 'number') { | ||
CliUx.ux.log(`Upload progress for ${name}: ${progressUpdate}`); | ||
async function walkDirectory(currentPath: string) { | ||
const entries = await fs.promises.readdir(currentPath, { withFileTypes: true }); | ||
for (const entry of entries) { | ||
const entryPath = path.join(currentPath, entry.name); | ||
if (entry.isDirectory()) { | ||
await walkDirectory(entryPath); | ||
} else { | ||
location = progressUpdate; | ||
const fileSize = (await fs.promises.stat(entryPath)).size; | ||
fileList.push({ | ||
fileName: path.relative(folderPath, entryPath), | ||
filePath: entryPath, | ||
fileSize, | ||
}); | ||
} | ||
} | ||
} | ||
|
||
if (location === '') throw ('no location returned'); | ||
|
||
const { files, ...rest } = platform as DesktopPlatform; | ||
const updatedPlatform = updatedPlatformEntries.find((item) => item.platform === name); | ||
if (!updatedPlatform) throw ("updated platform path not found"); | ||
await walkDirectory(folderPath); | ||
return fileList; | ||
} | ||
|
||
const fileStat = await fs.promises.stat(updatedPlatform.path); | ||
const downloadSize = fileStat.size.toString(); | ||
// Helper function for single file uploads | ||
async function getSingleFile(filePath: string): Promise<Array<{ fileName: string, filePath: string, fileSize: number }>> { | ||
const fileSize = (await fs.promises.stat(filePath)).size; | ||
const fileName = path.basename(filePath); | ||
|
||
meta.platforms[name as SupportedPlatform] = { | ||
...rest, | ||
name: preSignedUrl.fileName, | ||
external_url: `${baseGateWayURL}${location}`, | ||
downloadSize: downloadSize, | ||
installSize: downloadSize, | ||
installScript: updatedPlatform.installScript, | ||
}; | ||
} | ||
CliUx.ux.action.stop(); | ||
return meta; | ||
return [{ | ||
fileName, | ||
filePath, | ||
fileSize, | ||
}]; | ||
} |
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.
we need to be able to support web as well in the platform keys in the hyperplay.yml file
i.e. a hyperplay.yml file like
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.
might make sense to add an override variable in the platform config as well like
ignoreExecutableCheck
for a platform so this check is opt-out and a general solutionThere 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.
Re: my below message, I think
ignoreExecutableCheck
is a good to have but if it's not expected for a web or a webgl build, I don't believe the user should have to add it. I worry that the config is already getting a bit verbose, would a user have to do?Also as another thought, it may be bad to enable certain things as general solutions, for all of the native fields we want to enforce the executable, and it would actually cause a lot of problems for them to skip the check.