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

feat(cli): --project-name init option #29695

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

Conversation

nmussy
Copy link
Contributor

@nmussy nmussy commented Apr 2, 2024

Issue # (if applicable)

Closes #4865.

Reopened from #28979, which was automatically despite an exemption request being submitted

Reason for this change

Multiple requests have been made to allow the user to change the project name on cdk init, currently set to the working directory name, to a custom value.

Description of changes

This PR documents, implements and tests a new --project-name for the cdk init CLI. Its default value is the current behavior, i.e. the working directory name.

This PR follows up my previous attempt, now stale (#4884)

Description of how you validated changes

The new CLI option is being validated by a test added in init.test.ts

Checklist


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

@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 Apr 2, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team April 2, 2024 19:12
@github-actions github-actions bot added the distinguished-contributor [Pilot] contributed 50+ PRs to the CDK label Apr 2, 2024
if (this.languages.indexOf(language) === -1) {
error(`The ${chalk.blue(language)} language is not supported for ${chalk.green(this.name)} `
+ `(it supports: ${this.languages.map(l => chalk.blue(l)).join(', ')})`);
throw new Error(`Unsupported language: ${language}`);
}

const projectInfo: ProjectInfo = {
name: decamelize(path.basename(path.resolve(targetDirectory))),
name: projectName ?? decamelize(path.basename(path.resolve(targetDirectory))),
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any validation we need to do here? for example, are spaces allowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing is currently stopping a user from doing this:

mkdir "test folder with spaces"
cd "test folder with spaces"
npx cdk init --language=typescript

Which the CDK doesn't handle particularly well out of the box:
cdk.json

  "app": "npx ts-node --prefer-ts-exts bin/test folder with spaces.ts",
npm run cdk deploy

> test folder with spaces@0.1.0 cdk
> cdk deploy

node:internal/modules/cjs/loader:1147
  throw err;
  ^

Error: Cannot find module './test'

Manually quoting the path in "app" does seem to result in the deploy and destroy working

@msambol
Copy link
Contributor

msambol commented Apr 2, 2024

@nmussy Can you include a README update with this as well please?

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 2, 2024
@nmussy
Copy link
Contributor Author

nmussy commented Apr 3, 2024

Can you include a README update with this as well please?

The documentation of the init command is currently pretty bare bones:

### `cdk init`
Creates a new CDK project.
```console
$ # List the available template types & languages
$ cdk init --list
Available templates:
* app: Template for a CDK Application
└─ cdk init app --language=[csharp|fsharp|java|javascript|python|typescript]
* lib: Template for a CDK Construct Library
└─ cdk init lib --language=typescript
* sample-app: Example CDK Application with some constructs
└─ cdk init sample-app --language=[csharp|fsharp|java|javascript|python|typescript]
$ # Create a new library application in typescript
$ cdk init lib --language=typescript
```

All of the options are listed in init --help, I don't think this one is more deserving of a README inclusion than say --role-arn or --generate-only

Copy link
Contributor

@msambol msambol left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@TheRealAmazonKendra
Copy link
Contributor

Definitely closed due to our fault, not yours. We need to update our bot to not close PRs when the changes requested are because you're waiting on us. But also, we might have a bug because I thought I implemented that thing to not close when we have an exemption request or a clarification request. I could be misremembering, though. That was a hot minute ago.

@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. labels Apr 4, 2024
@TheRealAmazonKendra TheRealAmazonKendra added pr-linter/do-not-close The PR linter will not close this PR while this label is present and 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 Apr 4, 2024
@TheRealAmazonKendra
Copy link
Contributor

Oops. I used all the keywords the bot responds to. I only meant to add the do-not-close label.

@TheRealAmazonKendra
Copy link
Contributor

Anyway, I think I don't want to take this change as-is, not because your change isn't good but because of a previous one I (and others) made when making cdk migrate. We're verging a little on the side of spaghetti code here. Mistakes were made.

I'm doing some investigations that are tangentially related to this change to gimme a few days or so and I'll provide a meaningful response here with a solid path forward. But also, thanks for giving our init templates some attention. They need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distinguished-contributor [Pilot] contributed 50+ PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. pr-linter/do-not-close The PR linter will not close this PR while this label is present
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cdk init should have option to produce new project with fixed file and class names
4 participants