feat(cli): option to ignore no stacks by msambol · Pull Request #28387 · aws/aws-cdk

msambol · 2023-12-15T17:02:55Z

I'm new to development on this package—any feedback regarding testing is appreciated.

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

msambol · 2023-12-15T17:03:08Z

Still working through this, need to add tests.

aws-cdk-automation

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.

msambol · 2023-12-16T20:42:05Z

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli-lib.integtest.ts

+    });
+  }
+}));
+


should the integ tests go in this file?

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts

msambol · 2023-12-20T22:32:39Z

Exemption Request: I added a CLI integ test instead of framework-integ, because this is a change of CLI.

msambol · 2023-12-20T23:03:31Z

@lpizzinidev When you have time could mind reviewing?

lpizzinidev

The implementation looks good.
I left some comments for improvements 👍

lpizzinidev · 2023-12-21T16:37:38Z

packages/aws-cdk/lib/cli.ts

      .option('asset-parallelism', { type: 'boolean', desc: 'Whether to build/publish assets in parallel' })
-      .option('asset-prebuild', { type: 'boolean', desc: 'Whether to build all assets before deploying the first stack (useful for failing Docker builds)', default: true }),
+      .option('asset-prebuild', { type: 'boolean', desc: 'Whether to build all assets before deploying the first stack (useful for failing Docker builds)', default: true })
+      .option('ignore-no-stacks', { type: 'boolean', desc: 'Ignore the error message if the app contains no stacks', default: false }),


Suggested change

.option('ignore-no-stacks', { type: 'boolean', desc: 'Ignore the error message if the app contains no stacks', default: false }),

.option('ignore-no-stacks', { type: 'boolean', desc: 'Whether to deploy if the app contains no stacks', default: false }),

lpizzinidev · 2023-12-21T16:39:13Z

packages/aws-cdk/lib/cdk-toolkit.ts


    const startSynthTime = new Date().getTime();
-    const stackCollection = await this.selectStacksForDeploy(options.selector, options.exclusively, options.cacheCloudAssembly);
+    // eslint-disable-next-line max-len


Suggested change

// eslint-disable-next-line max-len

We should just take linter's advice and split the next line into multiple lines (same below).

lpizzinidev · 2023-12-21T16:39:44Z

packages/aws-cdk/lib/api/cxapp/cloud-assembly.ts

+      } else {
+        throw new Error('This app contains no stacks');
+      }


Suggested change

} else {

throw new Error('This app contains no stacks');

}

throw new Error('This app contains no stacks');

No need for an else

lpizzinidev · 2023-12-21T16:48:39Z

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli-lib.integtest.ts

  }
 }));

+integTest('cli-lib deploy no stack', withCliLibFixture(async (fixture) => {


We should also add the throwing case for coverage.

lpizzinidev · 2023-12-21T16:55:48Z

packages/@aws-cdk-testing/cli-integ/lib/with-cli-no-stacks.ts

+/**
+ * Higher order function to execute a block with a CliLib Integration CDK app fixture
+ */
+export function withCliLibIntegrationCdkApp<A extends TestContext & AwsContext>(block: (context: CliLibIntegrationTestFixture) => Promise<void>) {


Should use a different name than with-cli-lib

lpizzinidev · 2023-12-21T16:56:18Z

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts

    .rejects.toThrow('conditional-resource does not exist');
 }));

+integTest('deploy --ignore-no-stacks', withDefaultFixture(async (fixture) => {


Not sure about this.
We should verify that the app deploys when there are no stacks in it.

lpizzinidev

Can you please add back the integration tests in cli-lib.integtest.ts? (both happy and failure cases)
Then this will be good imo.

lpizzinidev

On second thought, the integration tests in cli.integtest.ts should provide enough coverage here.
Thanks for the changes 👍

aws-cdk-automation · 2024-01-09T17:51:32Z

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

aws-cdk-automation · 2024-01-10T00:08:50Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

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

mergify · 2024-01-10T16:38:42Z

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 · 2024-01-10T17:05:58Z

AWS CodeBuild CI Report

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

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

mergify · 2024-01-10T17:06:21Z

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).

blimmer · 2024-01-10T17:41:35Z

Thank you @msambol !!!

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Dec 15, 2023

aws-cdk-automation requested a review from a team December 15, 2023 17:03

github-actions bot added the star-contributor [Pilot] contributed between 25-49 PRs to the CDK label Dec 15, 2023

aws-cdk-automation previously requested changes Dec 15, 2023

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Dec 15, 2023

msambol force-pushed the 28371 branch from 0f16f7a to 587259e Compare December 16, 2023 20:41

msambol commented Dec 16, 2023

View reviewed changes

msambol had a problem deploying to test-pipeline December 16, 2023 20:42 — with GitHub Actions Failure

msambol commented Dec 16, 2023

View reviewed changes

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts Show resolved Hide resolved

msambol marked this pull request as ready for review December 20, 2023 22:31

msambol had a problem deploying to test-pipeline December 20, 2023 22:31 — with GitHub Actions Failure

aws-cdk-automation added 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 Dec 20, 2023

lpizzinidev suggested changes Dec 21, 2023

View reviewed changes

aws-cdk-automation removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Dec 21, 2023

msambol added 6 commits December 21, 2023 20:25

feat(cli): add option to ignore no stacks

e017f82

add tests

4f0d20e

remove unneeded test

03c1447

add feedback from luca

b42cb06

temp staging

c583e43

lets try this

044dd76

msambol force-pushed the 28371 branch from 4a34135 to 044dd76 Compare December 23, 2023 20:50

msambol added 2 commits December 23, 2023 14:53

fix

cb550c1

fix2

0090999

lpizzinidev suggested changes Dec 24, 2023

View reviewed changes

lpizzinidev approved these changes Dec 24, 2023

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 24, 2023

github-actions bot mentioned this pull request Jan 1, 2024

Monthly PR metrics report - Dec 2023 #28544

Closed

Merge branch 'main' into 28371

c37634b

kaizencc added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-cli-test-run This PR needs CLI tests run against it. labels Jan 9, 2024

kaizencc temporarily deployed to test-pipeline January 9, 2024 17:50 — with GitHub Actions Inactive

aws-cdk-automation added pr/needs-cli-test-run This PR needs CLI tests run against it. and removed pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Jan 9, 2024

kaizencc changed the title ~~feat(cli): add option to ignore no stacks~~ feat(cli): option to ignore no stacks Jan 9, 2024

kaizencc added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr/needs-cli-test-run This PR needs CLI tests run against it. labels Jan 10, 2024

kaizencc approved these changes Jan 10, 2024

View reviewed changes

Merge branch 'main' into 28371

93c8c07

mergify bot had a problem deploying to test-pipeline January 10, 2024 16:39 Failure

mergify bot merged commit 37c79b9 into aws:main Jan 10, 2024

msambol deleted the 28371 branch January 10, 2024 17:48

blimmer mentioned this pull request Apr 11, 2024

(cli): Provide a way to ignore "This app contains no stacks" #29797

Open

2 tasks

	.option('ignore-no-stacks', { type: 'boolean', desc: 'Ignore the error message if the app contains no stacks', default: false }),
	.option('ignore-no-stacks', { type: 'boolean', desc: 'Whether to deploy if the app contains no stacks', default: false }),

Conversation

msambol commented Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msambol commented Dec 15, 2023

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

msambol Dec 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

msambol commented Dec 20, 2023

Uh oh!

msambol commented Dec 20, 2023

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

lpizzinidev Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

lpizzinidev Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

lpizzinidev Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

lpizzinidev Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

lpizzinidev Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

lpizzinidev Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

lpizzinidev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Jan 9, 2024

Uh oh!

aws-cdk-automation commented Jan 10, 2024

Uh oh!

mergify bot commented Jan 10, 2024

Uh oh!

aws-cdk-automation commented Jan 10, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jan 10, 2024

Uh oh!

blimmer commented Jan 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

msambol commented Dec 15, 2023 •

edited

Loading

lpizzinidev left a comment •

edited

Loading