Repository navigation
support StreamingTransform scripts in init, scan, and deploy - #73
Conversation
…re deploying, Name matching is case-insensitive, matching the backend
jcatt-sf
left a comment
jcatt-sf
left a comment
There was a problem hiding this comment.
Thanks for this, the streaming support fits in cleanly and the SDK-mirroring comments make it easy to follow. I left a few inline comments. The first one (init failing partway on a bad or missing SDK config.yaml) is the only one I'd call a bug. The rest are questions or suggestions, mostly where this mirrors the SDK and we might want to be a bit stricter or warn.
| * as a YAML document so comments and formatting in the SDK file are kept. | ||
| */ | ||
| async function writeStreamingRunConfig(sourcePath: string, destinationPath: string): Promise<void> { | ||
| const doc = parseDocument(await fs.readFile(sourcePath, 'utf8')); |
There was a problem hiding this comment.
ensureStreamingTemplateExists runs before the copy, but it only checks the example entrypoint. By the time we get here the package dir has already been populated, so if <sdkDir>/config.yaml is missing (an odd editable install, or an SDK layout change) the user gets a raw ENOENT and a half-initialized package.
Also, parseDocument doesn't throw on malformed YAML, it collects problems in doc.errors, so a bad file either gets silently rewritten or fails later in toString() with an unclear error.
Could we check that config.yaml exists as part of ensureStreamingTemplateExists, and throw an SfError if doc.errors.length > 0?
| const DATA_CUSTOM_CODE_PATH = 'services/data/v63.0/ssot/data-custom-code'; | ||
| // `invokeOptions` only binds at v67.0, so deployment creation routes there when | ||
| // it is set (streaming scripts). Everything else, including status polling, stays on v63.0. | ||
| const DATA_CUSTOM_CODE_INVOKE_OPTIONS_PATH = 'services/data/v67.0/ssot/data-custom-code'; |
There was a problem hiding this comment.
Question: for streaming, the deployment is created on v67.0, but status polling and createDataTransform (with type: 'STREAMING') stay on v63.0. Have we confirmed v63.0 data-transforms accepts STREAMING and that v63.0 status returns deployments created on v67.0? If either doesn't, the failure only shows up after the zip is uploaded. If it's verified (the e2e requests.json suggests it might be), ignore this.
There was a problem hiding this comment.
Fair question. requests.json only records what the plugin sends, so it doesn't prove the server accepts it. The split mirrors the SDK's deploy_full, which creates on v67.0 when invokeOptions is set and polls and creates the transform on v63.0.
| * (`read_dlo_deltas()` / `read_dmo_deltas()` flags, and `write_dlo_deltas` / | ||
| * `write_dmo_deltas` targets resolved like the batch scan) and validates them. | ||
| */ | ||
| export async function scanFileStreaming(filePath: string): Promise<StreamingDataAccessLayerCalls> { |
There was a problem hiding this comment.
Nit: scanFileStreaming and fileIsStreaming aren't used anywhere in src/ (updateConfig calls visitFile/isStreaming directly), so only the tests exercise them. That's fine if keeping parity with the SDK's API is the goal. Otherwise we could drop them, or have updateConfig use them so the tests cover the real code path.
There was a problem hiding this comment.
fileIsStreaming is now used by the deploy-time check from #2. scanFileStreaming stays as the counterpart to the SDK's scan_file_streaming and is covered by the delta-validation tests. Happy to drop it if you'd rather.
No description provided.