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.

Sign up for GitHub

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

Jump to bottom

Add dotnet 6 runtime #5815

Merged
merged 2 commits into from
Apr 11, 2022
Merged

Add dotnet 6 runtime #5815

merged 2 commits into from
Apr 11, 2022

Conversation

yongliu-mdsol
Copy link
Contributor

Aws Lambda supports dotnet 6 runtime.
Adding same support for localstack

yongliu-mdsol
yongliu-mdsol commented Apr 6, 2022
View reviewed changes
.circleci/config.yml Outdated
@@ -85,6 +85,7 @@ jobs:
docker pull -q lambci/lambda:20191117-python3.6
docker pull -q lambci/lambda:20191117-dotnetcore2.0
docker pull -q lambci/lambda:dotnetcore3.1
docker pull -q lambci/lambda:dotnet6.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

waiting lambci/docker-lambda#359 to get merged

Copy link
Member

Choose a reason for hiding this comment

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

Hi there! Lambci is pretty dead right now. Until we have the lambda rework in place, it might be a good solution to use the repository of mlupine: https://hub.docker.com/layers/docker-lambda/mlupin/docker-lambda/dotnet6/images/sha256-fc3edf2e50307a5c81e4da7690f6cb6427819279081e1d9b16e52a294bf21738?context=explore , as we do already with python3.9.

Copy link
Member

Choose a reason for hiding this comment

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

We would need to adjust the image though, in lambda_executor.py:1596.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome. Thanks for the feedback. @dfangl
I will look into this today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally got this working.
@dfangl can you take a final review?

Copy link
Member

Choose a reason for hiding this comment

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

Please add the docker pull -q ... part back in for the mlupin image!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏻 added back

dfangl
dfangl requested changes Apr 7, 2022
View reviewed changes
Copy link
Member

@dfangl dfangl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

All in all, thanks for taking this into your hands and creating this PR. Please just change the Readme accordingly, and add the docker pull command to the circleci stage again, for the mlupin image.
FYI: If it is alright with you, I would like to rebuild the zip deployment package myself, and commit it to your branch with a signed commit before merge. I want to stress this is nothing against you personally, just for new contributors, I do not like to allow (potentially harmful) binary blobs to commit.

tests/integration/awslambda/functions/dotnet6/src/dotnet6/Readme.md Outdated
@@ -0,0 +1,49 @@
# AWS Lambda Empty Function Project
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to rework this readme (remove all the autogenerated stuff), with just a short notice on how to build the commited zip archive (should be something like dotnet lambda package, right?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, I updated the readme.

localstack/services/awslambda/lambda_executors.py Outdated
@@ -1593,7 +1593,9 @@ def docker_image_for_lambda(cls, lambda_function: LambdaFunction):
if runtime == "nodejs14.x" and docker_image == DEFAULT_LAMBDA_CONTAINER_REGISTRY:
# TODO temporary fix until lambci image for nodejs14.x becomes available
docker_image = "localstack/lambda-js"
if runtime == "python3.9" and docker_image == DEFAULT_LAMBDA_CONTAINER_REGISTRY:
elif (
runtime == "python3.9" or runtime == "dotnet6"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
runtime == "python3.9" or runtime == "dotnet6"
runtime in ["python3.9", "dotnet6"]

could possibly also work. No real official style guidelines for this though, you might as well leave it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this actually reads better. I updated it

@yongliu-mdsol
Copy link
Contributor Author

@dfangl thanks again for the quick feedbacks. I've addressed your comments.
Re recompling the zip file, I'm fine with that. Feel free to add any changes to make this PR better.

dfangl
dfangl approved these changes Apr 11, 2022
View reviewed changes
Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

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

LGTM now. Ready to merge once the build passes.

@dfangl dfangl merged commit 5b2633c into localstack:master Apr 11, 2022
@dfangl
Copy link
Member

dfangl commented Apr 11, 2022

Thanks for the PR, its merged 🎉
Please note, that there is some more work to be done for the pro version to also work, which I will take over.

@yongliu-mdsol
Copy link
Contributor Author

@dfangl if you don't mind, can you share the work you did for the pro version? so I can contribute more next time. :)

Also, can you make a release with dotnet6 changes? I need it for my projects.
Not sure if that's something @whummer can help too.

@dfangl
Copy link
Member

dfangl commented Apr 11, 2022

Its closed-source, but it includes building the localstack/lambda:dotnet6 image, which has pro-specific customization, and is needed when the pro version is used.
Modifications are already complete, so dotnet6 should now work with both pro and community.
In terms of release, they are normally bi-weekly, and usually not done out of cadence.
For now, as a workaround, I can offer you the docker functionality to pin the image to a hash value, by using localstack/localstack@sha256:0143fd3606c6a98af3a8c8fd3f5a04f5d9190aea414e66cb0905d84f7e33a65f (or which ever hash you want).

@adam-knights adam-knights mentioned this pull request Apr 12, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dfangl dfangl dfangl approved these changes

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

@thrau thrau Awaiting requested review from thrau

@alexrashed alexrashed Awaiting requested review from alexrashed alexrashed is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yongliu-mdsol @dfangl