feat(KNO-4367): Add pull email layout command #233
Conversation
| try { | ||
| if (indexDirCtx.exists) { | ||
| await fs.copy(indexDirCtx.abspath, backupDirPath); | ||
| await fs.emptyDir(indexDirCtx.abspath); |
There was a problem hiding this comment.
If we always blow out everything in the index directory, then we won't be able to preserve the extracted file paths used in the existing email layout dirs. Could we take the approach similar to workflows where we selectively remove any email layouts that aren't found in emailLayouts?
| }; | ||
|
|
||
| /* | ||
| * Validate the extracted file path based on its format and uniqueness (but not |
There was a problem hiding this comment.
Right now I don't see we are doing a uniqueness check here, and I think we should replicate that here as well?
And in that case, we might be able to abstract out the validateExtractedFilePath function also as a shared helper? Instead of EmailLayoutDirContext or WorkflowDirContext, it could take a sourceFileAbspath string?
There was a problem hiding this comment.
Great idea, here's the abstraction: ce50327
| // If not marked with the @suffix, there's nothing to do. | ||
| if (!FILEPATH_MARKED_RE.test(key)) return; | ||
|
|
||
| const objPathStr = key.replace(FILEPATH_MARKED_RE, ""); |
There was a problem hiding this comment.
| const objPathStr = key.replace(FILEPATH_MARKED_RE, ""); | |
| const inlineObjPathStr = ObjPath.stringify(parts).replace(FILEPATH_MARKED_RE, ""); |
I know a layout json object is currently a flat object (i.e. 1 depth), but I think it'd be good to consistently use the ObjPath helper here so we can extend to support a nested layout object in the future.
There was a problem hiding this comment.
Here's the commit where I improves this part of the code with all the nits you said: #233
There was a problem hiding this comment.
One last thing on the validateExtractedFilePath function signature, but otherwise this looks good to go - thanks @francoborr!
src/lib/marshal/shared/helpers.ts
Outdated
| export const validateExtractedFilePath = ( | ||
| val: unknown, | ||
| sourceFileAbspath: string, | ||
| sourceJson: string, |
There was a problem hiding this comment.
| sourceJson: string, |
In checkIfValidExtractedFilePathFormat, we refer to sourceFileAbspath as the abspath to the json file, so I would say we match that here as well?
Taking the directory context abspath and the json file name separately like you've done here works also, but in that case we probably should name change sourceFileAbspath arg above to something different like, dirContextAbspath. Otherwise we have the same name vars that hold/mean something different.
francoborr
deleted the
franco-kno-4367-cli-add-pull-email-layout-command
branch
Description
Adds a
layout pullcommand for pulling a given email layout into a directory. It also allows to pull all email layouts from a given environment using the--allflag along with the--layout-dir.It allows the following flags:
This PR also abstract some logic we use for reading external field contents. Before we used this logic when pulling
workflows, but now we use the same logic for pullinglayouts.Tasks
KNO-4367
Video
https://www.loom.com/share/d0035467d1cf48a999536b466d5dd1df