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

Updated the repo to work with the application load balancer. #17

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

Conversation

proggeramlug
Copy link

This includes everything needed to run Vapor apps through Lambda. I've left it as it is working for me now and it's open for discussion.

Some things, like config and the exact branches (pending merging of the swift-lambda-runtime) need to be adjusted but this way people can comment, suggest and improve things already. :)

@codecov
Copy link

codecov bot commented Jan 6, 2021

Codecov Report

Merging #17 (949dd64) into main (93cec68) will increase coverage by 0.15%.
The diff coverage is 17.39%.

❗ Current head 949dd64 differs from pull request most recent head 9e50652. Consider uploading reports for the commit 9e50652 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
+ Coverage   17.07%   17.22%   +0.15%     
==========================================
  Files           3        4       +1     
  Lines         205      296      +91     
==========================================
+ Hits           35       51      +16     
- Misses        170      245      +75     
Impacted Files Coverage Δ
Sources/VaporAWSLambdaRuntime/LambdaServer.swift 0.00% <0.00%> (ø)
Sources/VaporAWSLambdaRuntime/ALB.swift 17.97% <17.97%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93cec68...9e50652. Read the comment docs.

@Jonathan-Mckenzie
Copy link

Bummer this hasn't been merged into a 0.7.0 yet... We're just going to pull skelpo's vapor-aws-lambda-runtime instead...

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! A few changes to get it ready then we can merge

@@ -11,7 +11,7 @@ build_lambda:
$(SWIFT_DOCKER_IMAGE) \
swift build --product Hello -c release

package_lambda: build_lambda
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason this was removed?

@@ -0,0 +1,90 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

Again remove

@@ -0,0 +1,7 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

These files should be removed - I think if you rebase the current main on to yours it should pull in an updated gitignore with .swiftpm - if not could you add it and remove these?

@@ -16,7 +16,7 @@ let package = Package(
],
dependencies: [
.package(url: "https://github.com/apple/swift-nio.git", .upToNextMajor(from: "2.13.0")),
.package(url: "https://github.com/swift-server/swift-aws-lambda-runtime.git", .upToNextMajor(from: "0.3.0")),
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be changed back

nioHeaders.add(name: key, value: value)
}

/*if let cookies = req., cookies.count > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Lets remove this

}
}

ctx.logger.debug("The constructed URL is: \(url)")
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer this be at trace level since it's coming from the library - See the log level PR for what we're trying to move towards

@@ -79,13 +79,13 @@ public class LambdaServer: Server {
public enum RequestSource {
case apiGateway
case apiGatewayV2
// case applicationLoadBalancer // not in this release
case applicationLoadBalancer
Copy link
Member

Choose a reason for hiding this comment

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

🙌

@@ -0,0 +1,7 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

again remove

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

Successfully merging this pull request may close these issues.

3 participants