-
Notifications
You must be signed in to change notification settings - Fork 107
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
CUMULUS-3960: Updated PostToCmr task to be able to republish granules #3906
Conversation
.eslintrc.js
Outdated
@@ -62,6 +62,7 @@ module.exports = { | |||
}, | |||
globals: { | |||
JSX: true, | |||
AggregateError: false, |
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.
This is to resolve eslint error error 'AggregateError' is not defined
. Later versions of eslint have resolved the error.
const results = await pMap( | ||
updatedCMRFiles, | ||
(cmrFile) => publish2CMR(cmrFile, cmrSettings, cmrRevisionId), | ||
{ concurrency } |
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.
stopOnError: true
is default, if set to false
, it will change current lambda behavior
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.
Jonathan suggested to have the promise.allsettled behavior, which is adding stopOnError: true
to pMap, like
{ concurrency, stopOnError: true }, but that changes the current behavior of post-to-cmr, unit tests fail, and out of the scope of this ticket.
The comment explains why it's not added.
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.
ohhh ok, that makes sense, thanks for lmk 🙌
@@ -5,6 +5,6 @@ | |||
], | |||
"statements": 94.0, | |||
"functions": 88.0, | |||
"branches": 97.0, | |||
"branches": 88.0, |
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.
Only the handler function is not covered as before, not sure how to improve this.
…o jl/CUMULUS-3960
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.
Just some questions/comments I had, one or two small NITs, once answered + some local testing I'll approve 🙌
@@ -233,6 +243,17 @@ async function publish2CMR(cmrPublishObject, creds, cmrRevisionId) { | |||
throw new Error(`invalid cmrPublishObject passed to publis2CMR ${JSON.stringify(cmrPublishObject)}`); | |||
} | |||
|
|||
/** |
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.
is there a reason why this wasn't here before? did we never have the case of having to remove from CMR until now or was there just another way to do it (via API or something)
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.
mainly for my curiousity I guess
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.
api package has _removeGranuleFromCmr which does more stuff
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.
so this is like a mini-function of that kinda where it only deletes, sounds good and pretty useful maybe 👍
* @throws {Error} - Error from CMR request | ||
*/ | ||
async function removeGranuleFromCmr({ granules, cmrSettings, concurrency }) { | ||
const granulesToUnpublish = granules.filter((granule) => granule.published || !!granule.cmrLink); |
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.
its considered published if it doesnt have a granule.cmrLink
?
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.
In theory published granule should have cmrLink, but we should be able to delete the granule from cmr if we don't have the link
cmrClient.CMR.prototype.ingestGranule.restore(); | ||
}); | ||
|
||
await s3PutObject({ |
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.
I get the other calls/functions but what does this s3PutObject do in the case of the test?
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.
postToCMR function reads the cmr metadata file and ingest to CMR.
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 for addressing my questions and comments! LGTM 🚀 🎈
Summary: Summary of changes
Addresses CUMULUS-3960: Update CMR metadata with new collection info
Changes
PostToCmr
task to be able torepublish
granulesTesting
Note: The cmr concept-id remains the same in my test
PR Checklist