feat: port zip command from Python to native TypeScript#24
Conversation
Replace the spawn-based wrapper around `datacustomcode zip` with a native TypeScript implementation using `jszip`. The new `zipBuilder` module mirrors the Python logic in `datacustomcode/cli.py` and `deploy.py`: SDK config discovery, package-type detection, requirements detection, optional Docker dependency builder, and `deployment.zip` creation (excludes `.DS_Store`, omits implicit folder entries to match the Python `zipfile` output). Drops the Python/pip/binary environment check from the zip path since the command no longer shells out, and removes the now-unused `executeBinaryZip` and `DatacodeZipExecutionResult` exports. W-21770329
jcatt-sf
force-pushed
the
w-21770329-zip-ts-port
branch
from
c149aed to
7d99436
Compare
| if (entry.isDirectory()) { | ||
| return walk(full); | ||
| } | ||
| if (entry.isFile() && entry.name !== '.DS_Store') { |
There was a problem hiding this comment.
This seems to ignore symlinks by looking (only) for files? The python implementation does not as I went to some great lengths to deal with symlinks when this package get's unzipped.
| }) | ||
| ); | ||
| for (const entry of entries) { | ||
| archive.file(entry.portableName, entry.data, { |
There was a problem hiding this comment.
I don't know if this matters but I know Python zip preserves permissions such as executable. A quick google says this can be with Typescript as by passing:
unixPermissions: entryStat.mode & 0o777
| * | ||
| * Mirrors `prepare_dependency_archive` in `datacustomcode/deploy.py`. | ||
| */ | ||
| export async function prepareDependencyArchive( |
There was a problem hiding this comment.
I don't see any unit test coverage for this function, perhaps on purpose for some reason. Just pointing it out.
| * | ||
| * Mirrors `has_nonempty_requirements_file` in `datacustomcode/deploy.py`. | ||
| */ | ||
| export function hasNonemptyRequirementsFile(directory: string): boolean { |
There was a problem hiding this comment.
hasNonemptyRequirementsFile(directory) checks dirname(directory)/requirements.txt (parent-of-payload) — but prepareDependencyArchive then copies requirements.txt and
build_native_dependencies.sh from CWD, not from that resolved location. If CWD ≠ base directory, the existence check and the actual copy disagree.
| export type SharedResultProps = { | ||
| success: boolean; | ||
| pythonVersion: PythonVersionInfo; | ||
| pythonVersion?: PythonVersionInfo; |
There was a problem hiding this comment.
Curious why this became optional?
joroscoSF
left a comment
joroscoSF
left a comment
There was a problem hiding this comment.
A couple of changes requested and some comments for (my own) clarity.
- Restore `pythonVersion` as a required field on `SharedResultProps`. The zip command no longer needs a Python version, so `ZipResult` defines its own shape (matching how `scanBase` diverged from the shared props in PR salesforcecli#22). - Include file symlinks when collecting payload files so the archive matches the Python `os.walk` + `zipfile.write` behavior. Broken and directory symlinks are still skipped. - Pass `unixPermissions: stat.mode & 0o777` on each archive entry so executable bits survive the round trip. - Resolve `requirements.txt`, `build_native_dependencies.sh`, `payload/archives`, and `payload/py-files` from the resolved base directory rather than the current working directory. Run `docker build` with `cwd: baseDirectory` so `Dockerfile.dependencies` is found regardless of caller CWD. - Add a `DockerRunner` injection seam and unit tests covering `prepareDependencyArchive` for both script and function package types, plus tests for symlink handling and unix permission preservation. W-21770329
| const mountPath = tempDir.replace(/\\/g, '/'); | ||
| const args = ['run', '--rm', '-v', `${mountPath}:/workspace`, DOCKER_IMAGE_NAME]; | ||
| if (network !== 'default') { | ||
| args.push('--network', network); |
There was a problem hiding this comment.
If I'm reading this correctly the --network flag will be placed after the image name which is interpreted as the command or arguments for the command.
| ]); | ||
| }); | ||
|
|
||
| it('normalizes Windows-style backslashes in the mount path', () => { |
There was a problem hiding this comment.
Let's fix this test to ensure we catch the --network flag as a command/argument problem here.
| const packageType = await getPackageType(baseDirectory); | ||
|
|
||
| if (hasNonemptyRequirementsFile(baseDirectory)) { | ||
| await prepareDependencyArchive(baseDirectory, dockerNetwork, packageType, log); |
There was a problem hiding this comment.
This needs some verification logic to ensure the build files are present else we will get ENOENT or something similar
2 participants
Summary
datacustomcode zipwith a native TypeScript implementation insrc/utils/zipBuilder.ts(usesjszip).datacustomcode/cli.pyanddeploy.py: SDK-config discovery, package-type detection, requirements detection, optional Docker dependency builder, anddeployment.zipcreation (excludes.DS_Store, omits implicit folder entries to match the Pythonzipfileoutput).zipBasesince the command no longer shells out, and removes the unusedexecuteBinaryZip/DatacodeZipExecutionResultexports.W-21770329
Test plan
yarn test(compile + lint + tests) — 93 passing locally.zipagainst fresh script and function packages — produceddeployment.zipwith the same files Python produces.