Skip to content

Conversation

@chris-armstrong
Copy link
Contributor

@chris-armstrong chris-armstrong commented Apr 19, 2022

Add support for --loader and --external options to nodejs_npm_esbuild workflow.

This PR replaces #340 and #343, rebased against develop as requested.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@chris-armstrong chris-armstrong marked this pull request as ready for review April 19, 2022 05:49
@moelasmar moelasmar requested a review from mildaniel April 19, 2022 23:52
@moelasmar
Copy link
Contributor

thanks @chris-armstrong for you contribution. I see you already closed the PR #343, but kept PR #340 open, shall we check both of this PR, and #340 as well?

Copy link
Contributor

@moelasmar moelasmar left a comment

Choose a reason for hiding this comment

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

can you add integration test cases?

@chris-armstrong
Copy link
Contributor Author

can you add integration test cases?

I've added two new integration tests to cover the options, but the build is failing on all the esbuild integration tests because it can't find the esbuild binary. I didn't change this aspect of the tests, so are you able to help in diagnosing what is going on?

@chris-armstrong
Copy link
Contributor Author

can you add integration test cases?

I've added two new integration tests to cover the options, but the build is failing on all the esbuild integration tests because it can't find the esbuild binary. I didn't change this aspect of the tests, so are you able to help in diagnosing what is going on?

I'm going to assume the integration tests aren't run every time, and haven't been run for a while, as adding esbuild as a dependency to the one's that didn't have it has fixed the tests

@moelasmar
Copy link
Contributor

Thanks @chris-armstrong for the update. I will go through your code, and check the integration test cases.

@jfuss
Copy link
Contributor

jfuss commented May 4, 2022

@chris-armstrong It doesn't look this this PR has maintainer push access. Could you allow that (I think it is a check mark box on the right side of the page) or update the branch again? I can't merge this in without the branch being updated.

@chris-armstrong
Copy link
Contributor Author

@chris-armstrong It doesn't look this this PR has maintainer push access. Could you allow that (I think it is a check mark box on the right side of the page) or update the branch again? I can't merge this in without the branch being updated.

I've brought that up to date now @jfuss , I can't find the option to allow updating the branch

@jfuss jfuss merged commit 8a30554 into aws:develop May 5, 2022
@jfuss
Copy link
Contributor

jfuss commented May 5, 2022

@chris-armstrong You might have had to edit the PR. I forget exactly where/how. But thank you for updating the PR and making the contribution.

calavera pushed a commit to calavera/aws-lambda-builders that referenced this pull request Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants