CUMULUS-4534 -- Ecarton/multi provider db#4252
CUMULUS-4534 -- Ecarton/multi provider db#4252etcart wants to merge 61 commits intofeature/multi-providerfrom
Conversation
for more information, see https://pre-commit.ci
…ecarton/multi-provider_db
Jkovarik changed the title 
| t.deepEqual(targetMessage.meta.provider, fakeProvider); | ||
| }); | ||
| | ||
| test.serial('Sends an SQS message with default cmrProvider if not defined in collection', async (t) => { |
There was a problem hiding this comment.
We might want to reconsider this - if we're making CMR provider a thing that switches core behavior, we probably shouldn't have a default for collection cmr_providers. A default might be the right call for portions of the code that read - meaning for portions of the feature change-set, but I'd urge us not to allow it for migrating collections.
I realize modifying that puts us in feature branch territory as it can't be merged to main, and time is a consideration as well.
Thoughts?
There was a problem hiding this comment.
yea, my reflex is backwards compatibility so that we can roll it out more reasonably. can we remove default later? once we no longer need to play well with DAACs?
There was a problem hiding this comment.
My general inclination is to do the right thing for moving forward with consolidation here, given the code we're merging now shouldn't make it back into Legacy (assuming they've "frozen", which I believe they have).
The impact should be on us migrating, forcing collections to have a provider when they're migrated.... which is probably a good thing? The other downside that creates is needing to set one for the integration tests. The updside is it prevents accidentally importing a collection, testing it and sending it to the wrong CMR provider.
The technical risk is limited, so I'm willing to go with team consensus on it, but my vote would probably be to do the scaffolding work here to get a integration test provider in place and config our test collections with it.
One of the reasons I was asking about ticket scope is to understand where this concern lands in our ticketed work.
packages/api/lambdas/sf-scheduler.js Outdated
| collection, | ||
| provider, | ||
| }, | ||
| customMeta: merge( |
There was a problem hiding this comment.
Nit^H^H^H: Is there a reason to bring in lodash/merge over using a native spread operator?
Actually not a nit - lowdash merge mutates eventCustomMeta Given we're already doing `merge(messageTemplate.meta, customMeta, ...) further down the call stack, this probably isn't desirable over something like
customMeta: { ...eventCustomMeta, ...collectionCustomMeta, collection, provider, ...(collection.cmrProvider && { cmr: { provider: collection.cmrProvider }, }), } if we want to keep a default cmrProvider. Obviously there's some room for explicit/terse in construction of the object. The critical thing there also is that we don't wind up with 'cmr: { provider: undefined }' as I think you might run into issues with an explicit undefined overwriting the collection default
Not nit: I don't think we should merge collection.meta with meta like this - there's no contract around meta objects, and if we're tossing that because of consolidation we should properly namespace all of it, rather than merging it in this way.
Suggest: either pull a specific field from the collection.meta (it seems like we're adding meta.cmr.provider anyway, so already doing that?) only, unless we need something else from the collection meta duplicate stored in the message (we're sending collection already, e.g. meta.collection.meta).
There was a problem hiding this comment.
reason for lodash/merge rather than spreader is that it's recursive merge, and some properties we want to merge are at depth > 1
There was a problem hiding this comment.
but lemme look at the meta object...
There was a problem hiding this comment.
I don't remember why I added collectionCustomMeta.... I guess it just seemed right, I thought what I did was a part of keeping things the same with the switch to merge instead of ...spreader
There was a problem hiding this comment.
Apologies, I edited the comment after you'd updated- please note the mutation concern as well as the schema conflict.
| t.deepEqual(targetMessage.meta.cmr.provider, fakeMessageResponse.meta.cmr.provider); | ||
| }); | ||
| | ||
| test.serial('Sends an SQS message with cmrProvider as overridden by collection', async (t) => { |
There was a problem hiding this comment.
Consider addressing the edge cases re: merge if still relevant after resolving https://github.com/nasa/cumulus/pull/4252/changes#r2789097571
| @@ -0,0 +1,21 @@ | |||
| import { Knex } from 'knex'; | |||
| | |||
| export const up = async (knex: Knex): Promise<void> => { | |||
There was a problem hiding this comment.
Not sure it functionally matters given our likely number of collections, but might we want an index on this column given the odds of wanting to do queries grouping things by collection.provider?
| }); | ||
| } | ||
| }; | ||
| exports.config = { |
There was a problem hiding this comment.
Is this intentional given the small table/migration failure consequences?
There was a problem hiding this comment.
no, this is copy-pasted from another
There was a problem hiding this comment.
actually this is needed in order to index
packages/types/api/collections.d.ts Outdated
| createdAt?: number, | ||
| updatedAt?: number | ||
| updatedAt?: number, | ||
| cmrProvider?: string, |
There was a problem hiding this comment.
Nit: alpha sort. (not that this was set that way prior 😓 )
| | ||
| ### Added | ||
| | ||
| - **CUMULUS-4534** |
There was a problem hiding this comment.
One other, non-blocking thought - have we scoped doc updates in the tickets?
We need to communicate this change to the technical implementers
| table.dropColumn('cmr_provider'); | ||
| }); | ||
| } | ||
| await knex.raw('DROP INDEX IF EXISTS collection_cmr_provider_index'); |
There was a problem hiding this comment.
Nit: Is this a failsafe? I believe dropping the column should drop the index right?
There was a problem hiding this comment.
yea I hemmed and hawed on keeping the parity, but decided mainly on aesthetics to keep it :) I can pull it
…nto ecarton/multi-provider_db
5 participants
Summary: Summary of changes
Addresses CUMULUS-4534: Develop amazing new feature
Changes
PR Checklist
📝 Note:
For most pull requests, please Squash and merge to maintain a clean and readable commit history.