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

fix(bundling): enclosing metafile & tsconfig paths with quotes #32725

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

Conversation

aaarichter
Copy link

@aaarichter aaarichter commented Jan 3, 2025

Issue # (if applicable)

No

Reason for this change

Generating the metafile in a Windows environment can fail when the path are not enclosed with quotes.

Description of changes

What code changes did you make?

Enclosed paths the bundling arguments --metafile and --tsconfig

Have you made any important design decisions?

No

What AWS use cases does this change enable? To enable the use cases, which AWS service features are utilized?

Allows safe usage of metafile and tsconfig bundling options

Describe any new or updated permissions being added

<!— What new or updated IAM permissions are needed to support the changes being introduced ? -->

Description of how you validated changes

Have you added any unit tests and/or integration tests?

No, but when you have a project path with a whitespace, and use the metafile bundling argument, the metafile path should be passed correctly within quotes to esbuild.

I updated the existing tests.

Checklist


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

@aws-cdk-automation aws-cdk-automation requested a review from a team January 3, 2025 15:27
@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Jan 3, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

✅ A exemption request has been requested. Please wait for a maintainer's review.

@aaarichter aaarichter force-pushed the fix/eslint-parameter-enclose branch from 1cc72bc to f62e4dd Compare January 3, 2025 15:37
@aaarichter
Copy link
Author

I have no idea what to do

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.
PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

why doesn't this text say what integration file needs to be adjusted ?!!?

@aws-cdk-automation aws-cdk-automation added pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Jan 3, 2025
@aaarichter aaarichter requested a review from a team as a code owner January 3, 2025 20:20
@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jan 3, 2025
@aaarichter
Copy link
Author

@samson-keung reverted accidental changes. I'm not sure about the artifacts for/of the integration changes and how to generate these

@samson-keung
Copy link
Contributor

Thanks for updating the PR. LGTM. I have pinged the CLI team to review as well.

@samson-keung
Copy link
Contributor

@aaarichter I see two integ tests failed. The relevant log snippet:

@aws-cdk-testing/framework-integ: Snapshot Results: 
@aws-cdk-testing/framework-integ: Tests:    2 failed, 1043 total
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src1910170678/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-lambda-nodejs/test/integ.dependencies-bun.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src1910170678/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-lambda-nodejs/test/integ.function.js
@aws-cdk/aws-scheduler-targets-alpha: Verifying integration test snapshots...
@aws-cdk-testing/framework-integ: Error: Some tests failed!
@aws-cdk-testing/framework-integ: To re-run failed tests run: integ-runner --update-on-failed
@aws-cdk-testing/framework-integ:     at main (/codebuild/output/src1910170678/src/github.com/aws/aws-cdk/packages/@aws-cdk/integ-runner/lib/cli.js:192:19)
@aws-cdk-testing/framework-integ: Error: integ-runner exited with error code 1
@aws-cdk-testing/framework-integ: Tests failed. Total time (2m19.6s) | integ-runner (2m10.4s) | /codebuild/output/src1910170678/src/github.com/aws/aws-cdk/node_modules/jest/bin/jest.js (8.3s)

Are you able to run and update them? Instructions can be found in https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#testing

@mrgrain mrgrain removed the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jan 9, 2025
@mrgrain
Copy link
Contributor

mrgrain commented Jan 9, 2025

No cli integ tests needed. This was picked-up from this commit which was later reverted.

@xazhao xazhao removed pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run labels Jan 13, 2025
@aaarichter
Copy link
Author

hey @samson-keung sry for the late reply. I was trying to do the steps but the integration steps fail:

yarn integ test/aws-lambda/test/integ.lambda.js --update-on-failed
yarn run v1.22.22
$ integ-runner --language javascript test/aws-lambda/test/integ.lambda.js --update-on-failed

Verifying integration test snapshots...

  ERROR      aws-lambda/test/integ.lambda 0.002s
      ENOENT: no such file or directory, open '/Users/anri/repositories/oss/aws-cdk/packages/@aws-cdk/integ-runner/lib/recommended-feature-flags.json'

This is based on the latest commit from main...

I'm clueless about what to do in this case. 😭

@aaarichter aaarichter force-pushed the fix/eslint-parameter-enclose branch from e2a7db8 to f22f907 Compare January 15, 2025 15:58
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 15, 2025
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

✅ A exemption request has been requested. Please wait for a maintainer's review.

@samson-keung
Copy link
Contributor

samson-keung commented Jan 16, 2025

Hey @aaarichter. No problem, we are here to help unblock contributors!

Looks like after you updated to latest main branch, the integ tests no longer fails.

Also as I look into the integ test, I saw a similar fix has been before for the inject option:
https://github.com/aws/aws-cdk/pull/29561/files#diff-d3a78437ce44ac758a33052c1ce663976f465681199c2c85cc42b8b57832b08e

One more ask: Would you be able to update the test to cover your fix as well?

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.40%. Comparing base (8e9d7a0) to head (f22f907).
Report is 12 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #32725   +/-   ##
=======================================
  Coverage   81.40%   81.40%           
=======================================
  Files         223      223           
  Lines       13727    13727           
  Branches     2411     2411           
=======================================
  Hits        11175    11175           
  Misses       2274     2274           
  Partials      278      278           
Flag Coverage Δ
suite.unit 81.40% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
packages/aws-cdk 80.74% <ø> (ø)
packages/aws-cdk-lib/core 82.10% <ø> (ø)

@aaarichter aaarichter force-pushed the fix/eslint-parameter-enclose branch from f22f907 to be5852d Compare January 17, 2025 20:49
@aaarichter
Copy link
Author

@samson-keung I added files to the integration test similar to the "inject PR" you linked in your comment.
What I can't figure out is how the artifacts are created, which are required by the PR checks in GitHub ¯_(ツ)_/¯

(I mean in my project I just would run cdk synth and these artifacts get created, but within this project I don't see how)

@aaarichter aaarichter force-pushed the fix/eslint-parameter-enclose branch from be5852d to 7c355ac Compare January 17, 2025 20:54
@samson-keung
Copy link
Contributor

@aaarichter You will need to run the integ test in order for it to generate the artifacts. You can try the following commands:

cd packages/@aws-cdk-testing/framework-integ/test
yarn integ aws-lambda-nodejs/test/integ.function.ts --update-on-failed

After that, the artifacts should be updated. Note that if you have never ran integ tests for this repo before, have a read at https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md#integration-tests to get yourself set up.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 7c355ac
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants