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

Add lambda support #14

Merged
merged 27 commits into from
Feb 2, 2023
Merged

Conversation

rupal-bq
Copy link
Member

Description

  • converted es6 modules to commonJS
  • Added Dockerfile to create container image for lambda
  • downgraded ora to resolve errors

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
@rupal-bq rupal-bq marked this pull request as ready for review February 1, 2023 16:50
@rupal-bq rupal-bq requested a review from a team as a code owner February 1, 2023 16:50
Dockerfile Outdated Show resolved Hide resolved
Copy link
Member

@joshuali925 joshuali925 left a comment

Choose a reason for hiding this comment

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

some comments, didn't finish reviewing

.gitignore Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/arguments.js Outdated Show resolved Hide resolved
Dockerfile Outdated
WORKDIR ${FUNCTION_DIR}
# Copy in the build image dependencies
COPY --from=build-image ${FUNCTION_DIR} ${FUNCTION_DIR}
RUN ls ${FUNCTION_DIR}/
Copy link
Member

Choose a reason for hiding this comment

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

Is this ls required?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not required but helps to see if required files are present before deploying the image (added bcoz it helped me to debug, we can totally remove all ls commands)

Copy link
Member

Choose a reason for hiding this comment

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

Because it increases the image size and what is the final image size?

Copy link
Member

Choose a reason for hiding this comment

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

might be better to remove, i think it adds an unnecessary layer to image

Copy link
Member Author

Choose a reason for hiding this comment

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

removed. I am bit confused about size. On my local it shows 1.16GB but on ECR it shows 357.10MB.

image
image

Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
2. Run the command in from the directory where Dockerfile and opensearch-reporting-cli-1.0.0.tgz exists.
```
docker build -t opensearch-reporting-cli .
Copy link
Member

Choose a reason for hiding this comment

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

users might be running this directly after cloning this repo and run npm install, do we need to dockerignore node_modules?

or do we expect users create a new directory, download Dockerfile and .tgz separately, and run docker build?

Also does this line work in mac/windows?

Copy link
Member Author

@rupal-bq rupal-bq Feb 1, 2023

Choose a reason for hiding this comment

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

users might be running this directly after cloning this repo and run npm install, do we need to dockerignore node_modules?

Dockerfile is using opensearch-reporting-cli-1.0.0.tgz. If someone just clone repo & do npm install then docker build will throw error for not finding this tar file. We need to add dockerignore if we build from source.

do we expect users create a new directory, download Dockerfile and .tgz separately, and run docker build?

When we have this file on opensearch downloads page, we can add step to download tar file in Dockerfile so all user will need is to get Dockerfile and run build command. No need to clone the repo.

does this line work in mac/windows?

yes

Dockerfile Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
@joshuali925
Copy link
Member

also a note we might be able to use babel or babel+webpack to keep ES6 modules in src, babel will build CommonJS for lambda runtime. maybe a todo item in the future to improve code

Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Copy link
Member

@joshuali925 joshuali925 left a comment

Choose a reason for hiding this comment

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

minor comments

docs/examples/lambda_container_image.md Outdated Show resolved Hide resolved
docs/examples/lambda_container_image.md Show resolved Hide resolved
docs/examples/lambda_container_image.md Outdated Show resolved Hide resolved
docs/examples/lambda_container_image.md Outdated Show resolved Hide resolved
docs/examples/lambda_container_image.md Outdated Show resolved Hide resolved
docs/examples/lambda_container_image_from_source.md Outdated Show resolved Hide resolved
rupal-bq and others added 4 commits February 2, 2023 00:02
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Co-authored-by: Joshua Li <joshuali925@gmail.com>
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