-
Notifications
You must be signed in to change notification settings - Fork 106
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-175 : Dashboard providers not in sync with AWS providers #234
Conversation
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.
made some comments
}); | ||
const collections = new models.Collection(); | ||
return collections | ||
.scan() |
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 continue to use the ElasticSearch for returning results for the list endpoint, otherwise, none of our query parameters will work.
packages/api/endpoints/providers.js
Outdated
}); | ||
const providers = new models.Provider(); | ||
return providers | ||
.scan() |
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 continue to use the ElasticSearch for returning results for the list endpoint, otherwise, none of our query parameters will work.
packages/api/endpoints/rules.js
Outdated
}); | ||
const rules = new models.Rule(); | ||
return rules | ||
.scan() |
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 continue to use the ElasticSearch for returning results for the list endpoint, otherwise, none of our query parameters will work.
packages/api/lib/response.js
Outdated
@@ -98,5 +98,17 @@ function handle(event, context, authCheck, func) { | |||
return func(cb); | |||
} | |||
|
|||
function listResponse(event, results) { |
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 not needed if we are listing from elastic search
@abarciauskas-bgse could you also update the changelog? |
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.
Made 1 request for a very small change plus two comments that are not blocking the merge of this PR.
@@ -29,11 +29,12 @@ See [Cumulus README](https://github.com/cumulus-nasa/cumulus/blob/master/README. | |||
Running tests for kinesis-consumer depends on localstack. Once you have installed localstack, you can start it for dynamoDB only: | |||
|
|||
``` | |||
SERVICES=dynamodb localstack start | |||
LAMBDA_EXECUTOR=docker localstack start |
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.
Since we are running tests from the root level of the repo, we should add this to the main readme if necessary. Alternatively, we can add the env variable to the docker-compose.yml, to have it available as part of the docker-compose up local
command.
``` | ||
|
||
Then you can run tests locally via: | ||
|
||
```bash | ||
LOCALSTACK_HOST=localhost npm run test | ||
export DOCKERHOST=$(ifconfig | grep -E "([0-9]{1,3}\.){3}[0-9]{1,3}" | grep -v 127.0.0.1 | awk '{ print $2 }' | cut -f2 -d: | head -n1) |
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.
Looking at this, I don't think there is a way to run this locally with our main docker image. So I'm not sure anymore if we have to move this to the root level of the repo.
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 think the db-indexer tests need some work to get working from the root of the repo. I'm going to skip them for now in hopes of returning to get them running as part of the larger test suite. This could help testing other lambdas, especially those in @cumulus/api.
packages/api/lambdas/db-indexer.js
Outdated
return Promise.resolve(); | ||
} | ||
|
||
let stack = process.env.stackName; |
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 not reassigned later on and should be a const
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.
Good call 05c31a6
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.
💯 👏
Summary: Hooks up db-indexer lambda function to dynamoDB streams which replicates updates to elasticsearch. Update API endpoints to just use elastic search.
Addresses CUMULUS-175: DDashboard providers not in sync with AWS providers
Changes
Test Plan
<ELASTICSEARCH_DOMAIN>/_all/<collection|rule|provider>/_search?pretty=true