Jk/cumulus 3992 #3912
Jk/cumulus 3992 #3912
Conversation
| return returnCustomValidationErrors(res, body); | ||
| } | ||
|
|
||
| if (!process.env.system_bucket) { |
There was a problem hiding this comment.
Do we perhaps want to check these ENVs before we parse the payload?
packages/api/endpoints/granules.js
Outdated
| deconstructCollectionId(body.sourceCollectionId) | ||
| ); | ||
| // check collection returned | ||
| if (pgCollection === undefined) { |
There was a problem hiding this comment.
I believe the BaseModel get will throw, not return undefined?
There was a problem hiding this comment.
Great catch - this was a leftover from an earlier implementation approach.
| collection_cumulus_id: pgCollection.cumulus_id, | ||
| }); | ||
| query.select('granule_id'); | ||
| query.limit(body.batchSize); |
There was a problem hiding this comment.
This defaults to 100, right? Will delete this comment if confirmed 😄
There was a problem hiding this comment.
Yes, zod has it set to default at 100
packages/api/endpoints/granules.js
Outdated
| @@ -1229,6 +1379,7 @@ router.delete('/:collectionId/:granuleId', del); | |||
|
|
|||
| module.exports = { | |||
| bulkDelete, | |||
| bulkMoveCollection, | |||
There was a problem hiding this comment.
This is pretty minor/unimportant but would bulkChangeCollection or bulkUpdateCollection make more sense? I think "Move" is probably the most accurate but if I'm reading this endpoint for the first time and I don't know what the endpoint does, I feel like "MoveCollection" is less self-explanatory... I think.
There was a problem hiding this comment.
UI is important though, and naming things is hard.
I don't hate bulk change collection...... 🤔
and neither does a huddle audience. TODO.
packages/api/lib/rulesHelpers.js
Outdated
| */ | ||
| async function buildPayload(rule) { | ||
| // makes sure the workflow exists | ||
| const bucket = getRequiredEnvVar('system_bucket'); | ||
| const stack = getRequiredEnvVar('stackName'); | ||
| const workflowFileKey = workflows.getWorkflowFileKey(stack, rule.workflow); | ||
| const workflowFileKey = workflows.getWorkflowFileKey(stack, rule.workflow || ''); |
There was a problem hiding this comment.
Does this fix a specific issue where rule.workflow was coming in as undefined or something?
There was a problem hiding this comment.
Good question: No, it fixes the type assertions - rule.workflow can be undefined.
That said, it should probably be more properly fixed as such:
| })) | ||
| ); | ||
| // index PG Granules into ES | ||
| await Promise.all( |
There was a problem hiding this comment.
Yeah, bummer. Do we want to add any comments or move ES-related setup to a place that's easily fenced off and removed for the forward release?
There was a problem hiding this comment.
Hmm, yeah, probably that's a good QOL refactor for the main merge. TODO.
| }); | ||
|
|
||
| test.serial('bulkMoveCollection generates the proper payload and calls startExecution with it', async (t) => { | ||
| process.env.CUMULUS_VERSION = 'fakeVersion'; |
There was a problem hiding this comment.
nit: prefer realistic values
| }; | ||
| await bulkMoveCollection(req, res); | ||
| t.is('Unable to find state machine ARN for workflow someDumbWorkflow', res.boom.badRequest.firstCall.args[0], 'Error message should match catch for getJsonS3Object'); | ||
| }); |
There was a problem hiding this comment.
What about a test for a Collection with no Granules?
There was a problem hiding this comment.
Seems reasonable enough. 054cfa1 - may refactor/DRY that up a bit, but should cover the thought.
Jkovarik
changed the base branch from
release-18.5.x
to
feature/CUMULUS-3751
packages/api/endpoints/granules.js
Outdated
| }, | ||
| }); | ||
|
|
||
| input.cumulus_meta = { ...input?.template.cumulus_meta, ...input.cumulus_meta }; |
There was a problem hiding this comment.
is this meant to be ...input.template?.cumulus_meta? I'm not sure what's happening here if input is undefined
There was a problem hiding this comment.
Yes - good catch. Functionally it shouldn't happen, but the return from buildPayload isn't certain to return a template value in cases where the s3 object is 0 byte/null/etc.
Summary: Summary of changes
Addresses CUMULUS-3992
Changes
See ticket/comments for more context.
PR Checklist