move upgraded .zap file into zcl/ subdirectory#1717
Conversation
paulr34
requested review from
brdandu,
dhchandw,
ethanzhouyc and
tecimovic
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an option to skip a specified subdirectory (hardcoded as 'zcl') when recursively searching for '.zap' files to prevent re-processing previously upgraded files. However, the recursive call to findZapFiles fails to pass the skipSubdirectory argument, which prevents the exclusion from working correctly for nested subdirectories.
paulr34
force-pushed
the
feat/move-zap-to-zcl-subdir-on-upgrade
branch
from
26b45c8 to
c52d30e
Compare
| * @returns {string[]} Paths of all .zap files found. | ||
| */ | ||
| function findZapFiles(dir) { | ||
| function findZapFiles(dir, skipSubdirectory = null) { |
There was a problem hiding this comment.
Have we investigated why this was not an issue before and is an issue now? We should not be hardcoding things like this. What is the underlying issue?
There was a problem hiding this comment.
I agree with you. We should meet with all teams involved and discuss the best path forward.
At the moment, I see two options:
1.) Merge this PR now to unblock progress, then follow up with a separate PR once we've aligned on the long-term solution.
2.) Hold off on merging, meet with the team to determine where the path should be defined, understand why this behavior started occurring, and then implement a fully agreed-upon solution.
Either way, this PR is available if timing becomes a factor and we need to unblock work quickly.
There was a problem hiding this comment.
Compromise: put the path in apack.json for now.
That avoids hardcoding it in ZAP and doesn't require SDK changes immediately. ZAP can consume the path from apack.json, and we can revisit the long-term ownership of the path later if needed.
During the upgrade flow, write the upgraded .zap file into a zcl/ subdirectory relative to where the original lives, so the path changes from /config/app.zap to /config/zcl/app.zap. The subdirectory is created automatically if it doesn't exist. findZapFiles now accepts an optional skip-subdirectory parameter so that re-runs don't recurse into the output directory and pick up files that were already upgraded. TODO: make the subdirectory name configurable (e.g. via apack.json) instead of hardcoded. Co-authored-by: Cursor <cursoragent@cursor.com>
paulr34
force-pushed
the
feat/move-zap-to-zcl-subdir-on-upgrade
branch
from
c52d30e to
b199a30
Compare
|
closing this for now |
2 participants
During the upgrade flow, write the upgraded .zap file into a zcl/ subdirectory relative to where the original lives, so the path changes from /config/app.zap to /config/zcl/app.zap. The subdirectory is created automatically if it doesn't exist.
findZapFiles now accepts an optional skip-subdirectory parameter so that re-runs don't recurse into the output directory and pick up files that were already upgraded.
TODO: make the subdirectory name configurable (e.g. via apack.json) instead of hardcoded.