-
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
Update queue-pdrs task to use the message adapter #228
Conversation
cumulus/tasks/queue-pdrs/index.js
Outdated
* @return {undefined} | ||
**/ | ||
function handler(event, context, cb) { | ||
async function queuePdrs(event) { |
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 function needs a jsdoc
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.
Fixed.
@@ -38,6 +38,7 @@ | |||
"license": "Apache-2.0", | |||
"dependencies": { | |||
"@cumulus/common": "^1.0.1", | |||
"@cumulus/cumulus-message-adapter-js": "0.0.1-beta.3", |
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.
We should probably have this on 1.0.0 now.
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.
}); | ||
}); | ||
|
||
test.todo('An appropriate error is thrown if the message template could not be fetched'); |
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 ticket for this?
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 now - https://bugs.earthdata.nasa.gov/browse/CUMULUS-368
const message = await getMessageFromTemplate(parsePdrMessageTemplateUri); | ||
|
||
message.meta.provider = provider; | ||
message.meta.collection = collection; | ||
|
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.
should we add message.meta.pdr = pdr here?
So when a granule is ingested, the granule is inserted into elastic search with pdr name.
https://github.com/cumulus-nasa/cumulus/blob/master/packages/api/es/indexer.js#L389
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've created https://bugs.earthdata.nasa.gov/browse/CUMULUS-373 for this.
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.
Also need to update the changelog.md for both PRs and do a version update
|
||
t.is(messages.length, 2); | ||
|
||
const receivedPdrnames = messages.map((message) => message.payload.pdr.name); |
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.
Nit: (Here and elsewhere in this pr)
If your function takes a single argument and doesn’t use braces, omit the parentheses
https://github.com/airbnb/javascript#arrows--one-arg-parens
But this is not consistent across the repo, so I don't feel strongly about leaving as is.
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.
Airbnb's guide says to omit the braces, but our project style guide overrides that:
https://github.com/cumulus-nasa/cumulus/blob/master/.eslintrc.json#L95
// Make sure we did receive those messages | ||
messages.forEach((message) => { | ||
const pdrName = message.payload.pdr.name; | ||
t.deepEqual(message, expectedMessages[pdrName]); |
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.
These tests are 💯
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.
PR looks good to me, however there should be testing steps and results
No description provided.