-
Notifications
You must be signed in to change notification settings - Fork 10
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
MWPW-135444 Integrate Franklin's bulk preview and publish API #60
Conversation
Added a bulkPreviewPublish / bulkJobStatus that uses the franklin bulkPreview and
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.
@raga-adbe-gh Looks good. One question about the PUBLISH call.
As discussed offline, we should test this with a larger set to understand the delay/wait that the API adds if a job goes beyond 15min. Given the batched approach with only 150 pages called per API call, I would assume we should be good on PROMOTE.
But the number of paths will be higher with bulk preview API call on COPY action ~500-1000 and those are not batched on our end. So we should definitely test that scenario.
actions/promote/worker.js
Outdated
@@ -179,7 +179,7 @@ async function promoteFloodgatedFiles(doPublish, batchManager, appConfig) { | |||
let previewStatuses = []; | |||
let publishStatuses = []; | |||
if (ENABLE_HLX_PREVIEW) { | |||
previewStatuses = await previewOrPublishPages(PREVIEW); | |||
previewStatuses = await bulkPreviewOrPublish(PREVIEW); |
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.
There is another call happening for PUBLISH. I would assume, that should call the new API as well?
milo-fg/actions/promote/worker.js
Line 189 in b458ed9
publishStatuses = await previewOrPublishPages(PUBLISH); |
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.
Thank you @sukamat . As suggested updated code with bulk publish api.
Update to use bulk publish api
For svg, pdf file the extension needs to be passed in preview/publish api.
Franklin API updates
actions/helixUtils.js
Outdated
logger.info(`${operation} progress ${JSON.stringify(jobStatusJson.progress)}`); | ||
jobStatusJson.data?.resources?.forEach((rs) => { | ||
if (operation === LIVE) { | ||
bulkPreviewStatus[rs.webPath] = { success: JOB_STATUS_CODES.includes(rs.status) }; |
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.
@raga-adbe-gh The Publish Job Status API has been updated to use path
. So we can remove this separate check.
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.
Thanks @sukamat have updated the PR and also merged fgColor changes
# Conflicts: # actions/copy/worker.js # actions/helixUtils.js
Upated missed out await while clearing state.
Added a bulkPreviewPublish / bulkJobStatus that uses the franklin bulk apis.
Env-
Tested with cc-pink -
Preview - 150 files - https://admin.hlx.page/job/adobecom/cc-pink/main/preview/job-2023-10-07t17-00-33-769z
Publish - https://admin.hlx.page/job/adobecom/cc-pink/main/publish/job-2023-10-11t03-58-25-403z