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

feat: enable pnpx with cdk init #30661

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: enable pnpx with cdk init #30661

wants to merge 1 commit into from

Conversation

IanKonlog
Copy link
Contributor

Issue # (if applicable)

Closes #23205.

Reason for this change

cdk init does not use pnpm install when creating a new project with pnpx cdk init app --language typescript

Description of changes

  • Add support for pnpm install by resolving the package manager tool used with the cdk init command.
  • Update cdk.json to use a place holder for the app command depending on the package manager tool used
  • Keep the default option as npm install in case nothing is passed

Description of how you validated changes

Manual testing

Checklist

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

@IanKonlog IanKonlog added the pr/do-not-merge This PR should not be merged at this time. label Jun 25, 2024
@github-actions github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jun 25, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team June 25, 2024 12:52
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jun 25, 2024
aws-cdk-automation
aws-cdk-automation requested changes Jun 25, 2024
View reviewed changes
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 has failed. See the aws-cdk-automation comment below for failure reasons. 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.

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jun 25, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: ecd757e
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

comcalvi
comcalvi requested changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@comcalvi comcalvi 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 a CLI integ test? It would be something like init-typescript-app-npx, they live here: https://github.com/aws/aws-cdk/tree/main/packages/%40aws-cdk-testing/cli-integ

packages/aws-cdk/lib/init.ts
Comment on lines +370 to +372
print(`Got exec path ${chalk.green(`${process.env.npm_execpath}`)}...`);
print(`Got command ${chalk.green(`${'pnpm'}`)}...`);
return 'pnpm';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
print(`Got exec path ${chalk.green(`${process.env.npm_execpath}`)}...`);
print(`Got command ${chalk.green(`${'pnpm'}`)}...`);
return 'pnpm';
return 'pnpm';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding it and testing as we speak

scanlonp
scanlonp reviewed Jun 25, 2024
View reviewed changes
Copy link
Contributor

@scanlonp scanlonp left a comment

Choose a reason for hiding this comment

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

So we are supporting pnpm. Where is the line for other package managers? We have custom logic for calling pnpx, but what about the next tool?

Would it be better to draw the line somewhere and create custom logic for each manager, or is there a way to map the command we get in to the command we call in init?

packages/aws-cdk/lib/init.ts
@@ -153,10 +153,14 @@ export class InitTemplate {

private async installProcessed(templatePath: string, toFile: string, language: string, project: ProjectInfo) {
const template = await fs.readFile(templatePath, { encoding: 'utf-8' });
await fs.writeFile(toFile, this.expand(template, language, project));
let appCommandTool = 'npx';
if (toFile.includes('cdk.json') && await getCommand() === 'pnpm') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is hard-coded for pnpm, but we wont support yarn or other package managers etc. Is that the approach we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

per my understanding we do not want to extend this feature pass pnpm following the comments in the original issue I think we should just expand the support to pnpm. Otherwise we expose ourselves into making changes every time there is a possibility of having a new package manager. Any thoughts?

mrgrain
mrgrain reviewed Jun 25, 2024
View reviewed changes
packages/aws-cdk/lib/init.ts
@@ -359,6 +364,16 @@ async function postInstallTypescript(canUseNetwork: boolean, cwd: string) {
}
}

async function getCommand(): Promise<string> {
print('Getting command...');
if (process.env.npm_execpath && process.env.npm_execpath.includes('pnpm')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (process.env.npm_execpath && process.env.npm_execpath.includes('pnpm')) {
if (process.env.npm_execpath?.includes('pnpm')) {

@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to a test file.
❌ Features must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/30661/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

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.

1 similar comment
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to a test file.
❌ Features must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/30661/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

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.

This was referenced Jul 1, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scanlonp scanlonp scanlonp left review comments

@mrgrain mrgrain mrgrain left review comments

@comcalvi comcalvi comcalvi requested changes

@aws-cdk-automation aws-cdk-automation aws-cdk-automation requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 pr/do-not-merge This PR should not be merged at this time. pr/needs-cli-test-run This PR needs CLI tests run against it.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(cdk-cli): (Does't use pnpm to install dependencies when invoked with pnpx)
5 participants
@IanKonlog @aws-cdk-automation @mrgrain @comcalvi @scanlonp