Skip to content
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

EDSC-4351: Implements AWS CDK #1849

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

EDSC-4351: Implements AWS CDK #1849

wants to merge 8 commits into from

Conversation

macrouch
Copy link
Contributor

Overview

What is the feature?

Implements AWS CDK

What areas of the application does this impact?

New deployment process, new local environment

Testing

Local env testing: follow README updates to set up local env, general testing of app

Checklist

  • I have added automated tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 40.14085% with 85 lines in your changes missing coverage. Please review.

Project coverage is 84.03%. Comparing base (df18a98) to head (3913d07).

Files with missing lines Patch % Lines
serverless/src/util/mockStepFunction.js 8.23% 71 Missing and 7 partials ⚠️
serverless/src/util/getQueueUrl.js 80.95% 3 Missing and 1 partial ⚠️
serverless/src/submitRetrieval/handler.js 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1849      +/-   ##
==========================================
- Coverage   93.38%   84.03%   -9.35%     
==========================================
  Files         778      779       +1     
  Lines       19022    19064      +42     
  Branches     4911     4920       +9     
==========================================
- Hits        17764    16021    -1743     
- Misses       1168     2907    +1739     
- Partials       90      136      +46     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bin/invokeLocal.mjs Outdated Show resolved Hide resolved
bin/start.js Outdated Show resolved Hide resolved
bin/deploy-bamboo.sh Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
"postplaywright": "npx nyc report --reporter=json --reporter=lcov --reporter=clover --report-dir=playwright-coverage",
"deploy-infrastructure": "cd cdk/earthdata-search-infrastructure && npm ci --include=dev && npm run cdk deploy -- --progress events --require-approval never",
"deploy-application": "cd cdk/earthdata-search && npm ci --include=dev && npm run cdk deploy -- --progress events --require-approval never",
"deploy-static": "cd cdk/earthdata-search-static && npm ci --include=dev && npm run cdk deploy -- --progress events --require-approval never"
},
"devDependencies": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm install --include=optional sharp
npm i @rollup/rollup-darwin-x64

I needed to install these two locally to get my env to work. The rollup I think was a originally coming in from serverless framework

}))

// Wait for 5 seconds before polling again
setTimeout(pollMessages, 5000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any performance issues with continuously polling the queues?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I have noticed, but it is an optional piece to run the application locally. The majority of things we do we don't need to run SQS at all

Copy link
Contributor

@eudoroolivares2016 eudoroolivares2016 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested that:

  • Ensure basic application start up etc
  • Ensure invoking a lambda works
  • Ensure we can use the admin pages
  • Ensure that new code changes get picked up
  • Step functions work
  • Redis cache works
  • Admin fails if you change the admin list locally
  • Local s3 works
    All worked locally

@dpesall
Copy link
Member

dpesall commented Jan 28, 2025

Changes look good, but it looks like CodeCov says it's still missing coverage on a few files. Approved under the assumption that these will be covered.

@macrouch
Copy link
Contributor Author

Changes look good, but it looks like CodeCov says it's still missing coverage on a few files. Approved under the assumption that these will be covered.

The only file the report is calling out that has significant changes is serverless/src/util/mockStepFunction.js, which is only used in development, just like the new scripts in bin. I included it in serverless/src/util because it felt weird to import a file from that bin directory into a lambda handler. We don't worry about testing any of those local dev files, so I don't want to spend time testing this file either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants