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 fix for cfn-response code for nodejs custome resource #10408

Merged
merged 3 commits into from Mar 12, 2024

Conversation

pinzon
Copy link
Member

@pinzon pinzon commented Mar 6, 2024

Motivation

Custom resource models backed by nodejs lambda are experiencing deployment issues as a result of the nodejs cfn-response file containing an additional line ending character within the container, and the request is directing traffic to port 443.

Changes

  • convert standard string to raw string to avoid extra \n character in the container code
  • the request for the insertion of the resource deployment into the bucket uses url port not directly 443

Testing

The custom resource are not able to be deployed on this project so there is no need for a test

@pinzon pinzon added the semver: patch Non-breaking changes which can be included in patch releases label Mar 6, 2024
Copy link

github-actions bot commented Mar 6, 2024

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 28m 35s ⏱️ + 1m 11s
2 692 tests  - 1  2 436 ✅  - 1  256 💤 ±0  0 ❌ ±0 
2 694 runs   - 1  2 436 ✅  - 1  258 💤 ±0  0 ❌ ±0 

Results for commit a33b3a6. ± Comparison against base commit 87f747f.

This pull request removes 2 and adds 1 tests. Note that renamed tests count towards both.
tests.aws.services.s3.test_s3.TestS3PresignedUrl ‑ test_s3_get_response_case_sensitive_headers[False]
tests.aws.services.s3.test_s3.TestS3PresignedUrl ‑ test_s3_get_response_case_sensitive_headers[True]
tests.aws.services.s3.test_s3.TestS3PresignedUrl ‑ test_s3_get_response_case_sensitive_headers

♻️ This comment has been updated with latest results.

@pinzon pinzon changed the title add fix cfn-response file for nodejs cfn custome resource add fix for cfn-response code for nodejs custome resource Mar 7, 2024
@coveralls
Copy link

Coverage Status

coverage: 83.408% (-2.5%) from 85.881%
when pulling a33b3a6 on lambda/patch/cfn-response-raw
into 87f747f on master.

@pinzon pinzon marked this pull request as ready for review March 8, 2024 19:41
Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

LGTM, in terms of testing this is actually covered in -ext. I think currently we only cover the python one, but we should extend this to have both a python-based as well as a NodeJS based custom resource test 👍

@@ -189,7 +189,7 @@ def send(event, context, responseStatus, responseData, physicalResourceId=None,
var parsedUrl = url.parse(event.ResponseURL);
var options = {
hostname: parsedUrl.hostname,
port: 443,
port: parsedUrl.port, // Modified line: LS uses port 4566 for https; hard coded 443 causes error
Copy link
Member

Choose a reason for hiding this comment

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

by default localstack/localstack-pro will actually use 443 but that should be fine as it extracts it from the url anyway now 👍

@pinzon pinzon merged commit 7d8f4e5 into master Mar 12, 2024
30 of 31 checks passed
@pinzon pinzon deleted the lambda/patch/cfn-response-raw branch March 12, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants