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-lib): [JS/TS only] experimental support for programmatic CLI api #22836

Merged
merged 3 commits into from
Jan 18, 2023

Conversation

mrgrain
Copy link
Contributor

@mrgrain mrgrain commented Nov 8, 2022

Experimental wrapper around the existing aws-cdk package, which is possible due to minor adjustments in the cli.

The API interface is adapted from cdk-cli-wrapper. Currently all options are passed as array args to the existing cli args parser function. In future all code from aws-cdk should move over here and the dependency between the two would be reversed. When this is done we can start refactoring the internal APIs to be more programmatically usable.

What's in a name

Currently aiming for @aws-cdk/cli-lib, but any suggestions are welcome.

Why is this bundled?

To make the package a jsii library we need to bundle non-jsii dependencies. For now most of the code is still located in the aws-cdk package which has a different build pipeline to our other packages, simply including the directory in bundledDependencies is not an option. While the aim is to eventually have most code in this package, we are not there yet. Until then, bundling is the easiest option.

The build setup as configured for this package is needed because jsii MUST operate on TypeScript files and jsii-pacmak MUST be used because this is how the various language packages are created. Therefore bundling must happen after the jsii assembly has been created, but before jsii-pacmak runs.

We should not import submodules that are not explicitly exported

Yes, that's right we should not do this and no user land code should do this.
In this situation there are two things that make this "okay":

  • It's our package and we are in a monorepo. The danger of importing submodules is that they can break at any time, as they are usually considered implementation detail. However with the monorepo, the build would fail on any change that's not reflected in both packages.
  • It's supposed to be temporary. I plan to follow-up with a code move PR very quickly. Ideally this situation will only last for a couple releases.

All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

@gitpod-io
Copy link

gitpod-io bot commented Nov 8, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team November 8, 2022 19:24
@github-actions github-actions bot added the p2 label Nov 8, 2022
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 8, 2022
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.

@mrgrain mrgrain added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Nov 8, 2022
@mrgrain mrgrain force-pushed the feat/cli-lib branch 2 times, most recently from 4daabfc to 746c163 Compare November 8, 2022 19:36
@@ -0,0 +1,237 @@
import * as core from '@aws-cdk/core';
import { exec as runCli } from 'aws-cdk/lib';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minimal example to proof this works with the bundled package:

npm install aws-cdk 
node -e "const { cli } = require('aws-cdk/lib'); cli()"

cli is the existing export. I have no reason to believe exec would behave any different. Any further proof is very tricky within the constraints of the build setup (the bundled version of aws-cdk is only created at package time and as a tarball).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally confirmed this by taking the produced tarball from our test build pipeline and used it in an app.

@mrgrain mrgrain added the pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested label Nov 10, 2022
@aws-cdk-automation aws-cdk-automation dismissed their stale review November 10, 2022 16:16

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

packages/@aws-cdk/cli-lib/lib/cli.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/cli-lib/package.json Show resolved Hide resolved
packages/@aws-cdk/cli-lib/package.json Outdated Show resolved Hide resolved
packages/@aws-cdk/cli-lib/package.json Outdated Show resolved Hide resolved
Comment on lines +200 to +209
try {
const templatesDir = path.join(rootDir(), 'lib', 'init-templates');
const templateNames = await listDirectory(templatesDir);
const templates = new Array<InitTemplate>();
for (const templateName of templateNames) {
templates.push(await InitTemplate.fromName(templatesDir, templateName));
}
resolve(templates);
} catch {
resolve([]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code should not fail if the template directory cannot be found.

@mrgrain mrgrain added the pr/do-not-merge This PR should not be merged at this time. label Jan 13, 2023
packages/@aws-cdk/cli-lib/lib/cli.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/cli-lib/package.json Outdated Show resolved Hide resolved
### list

```ts
// await this asynchronous method call using a language feature
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops! Missing Rosetta feature here I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes!

@mrgrain mrgrain marked this pull request as draft January 14, 2023 17:10
@mrgrain mrgrain force-pushed the feat/cli-lib branch 7 times, most recently from cd999c2 to 47bd963 Compare January 17, 2023 15:21
@mrgrain mrgrain marked this pull request as ready for review January 17, 2023 15:23
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Provisionally approved after you consider my questions

* }
* ```
*/
produce(context: Record<string, any>): Promise<string>
Copy link
Contributor

Choose a reason for hiding this comment

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

No environment?

If this library is used via jsii, then the process.env.CDK_DEFAULT_ACCOUNt = '1234'; statements will be executed in the Node process, which is separate from the JVM so System.getEnvironment('CDK_DEFAULT_ACCOUNT') will not find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well that sucks. Now I have to think gain. 😭

Copy link
Contributor

Choose a reason for hiding this comment

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

Just pass the environment in and let the user deal with it.

/**
* Create the CLI from a CloudAssemblyDirectoryProducer
*/
public static fromCloudAssemblyDirectoryProducer(producer: ICloudAssemblyDirectoryProducer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we drop the fromCloudAssemblyDirectory constructor on purpose?

Copy link
Contributor Author

@mrgrain mrgrain Jan 17, 2023

Choose a reason for hiding this comment

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

Yes, I think there's no point to it. If someone really wants to do this, they can do a stub producer:

{
  produce: async () => '/path/to/assembly/dir'
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Ya but it looks nicer

Copy link
Contributor Author

@mrgrain mrgrain Jan 17, 2023

Choose a reason for hiding this comment

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

But it's pretty useless! We can always add it later if people are asking for it. 🤷🏻‍♂️

@rix0rrr rix0rrr changed the title feat(cli-lib): experimental support for programatic CLI api feat(cli-lib): experimental support for programmatic CLI api Jan 18, 2023
@mrgrain mrgrain changed the title feat(cli-lib): experimental support for programmatic CLI api feat(cli-lib): [JS/TS only] experimental support for programmatic CLI api Jan 18, 2023
@mrgrain mrgrain removed the pr/do-not-merge This PR should not be merged at this time. label Jan 18, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 18, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Jan 18, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 0b6b716 into aws:main Jan 18, 2023
@mrgrain mrgrain deleted the feat/cli-lib branch January 18, 2023 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants