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: list stack dependencies #28995

Merged
merged 50 commits into from
Mar 5, 2024
Merged

feat: list stack dependencies #28995

merged 50 commits into from
Mar 5, 2024

Conversation

vinayak-kukreja
Copy link
Contributor

@vinayak-kukreja vinayak-kukreja commented Feb 6, 2024

Reason for this change

Existing cdk list functionality does not display stack dependencies. This PR introduces that functionality.

For instance,
Existing functionality:

❯ cdk list
producer
consumer

Feature functionality:

❯ cdk list --show-dependencies
- id: producer
  dependencies: []
- id: consumer
  dependencies:
    - id: producer
      dependencies: []

Description of changes

Changes are based on internal team design discussions.

  • A new flag --show-dependencies is being introduced for list cli command.
  • A new file list-stacks.ts is being added.
    • Adding listStacks function within the file for listing stack and their dependencies using cloud assembly from the cdk toolkit.

Description of how you validated changes

  • Unit and Integration testing

Checklist

Co-Author

Co-authored-by: @SankyRed


NOTE: We are currently getting it reviewed by UX too so the final display output might change.


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 February 6, 2024 07:03
@github-actions github-actions bot added the p2 label Feb 6, 2024
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Feb 6, 2024
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.

Comment on lines 79 to 80
```console
$ # List all stacks in the CDK app 'node bin/main.js'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to update these examples once CLI team decides on the UX for this command.

@vinayak-kukreja vinayak-kukreja added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Feb 7, 2024
@vinayak-kukreja
Copy link
Contributor Author

Exemption request:

Since this is a new flow path, the integration test for CLI would not make sense here. Integ tests would be added once we have the other flow path ready.

vinayak-kukreja and others added 6 commits February 8, 2024 14:26
Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
@vinayak-kukreja vinayak-kukreja marked this pull request as ready for review February 8, 2024 22:12
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 8, 2024
@vinayak-kukreja
Copy link
Contributor Author

Exemption Request:

We are cli integ testing this change which would not generate a snapshot.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 27, 2024
Copy link
Contributor

@colifran colifran left a comment

Choose a reason for hiding this comment

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

Looks great! I just have a few small comments.

});
}
} else {
data.dependencies?.push({
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should this be optional? Doesn't data always have a dependencies array even if it is just empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, good point, let me recheck and get back to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear I just mean I think this could just be data.dependencies.push(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. Removing this. Good catch 💯


const depStack = assembly.stackById(dependencyId);

if (depStack.stackArtifacts[0].dependencies.length > 0 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious -- why are we only getting the first stackArtifact here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we are getting the dependent stack by ID: assembly.stackById(dependencyId);, so there must be just one unique stack by id.

I can add a comment explaining this or/and initially I had an error if it was greater than one but I believed that validation should be done before this (I can check if that is present). Thougths?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh okay that makes sense. I think maybe just a comment here would be good. It wasn't totally clear to me right away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so stackById uses getStackArtifact which would always return one artifact but stackById wraps it in an array.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification. Not now, unrelated, and also not sure what the impact would be, but maybe this is something we can update in a future PR. It feels like stackById should return a single stack, not an array containing a single stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree :)

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Mar 1, 2024
vinayak-kukreja and others added 2 commits March 1, 2024 13:30
Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
@w-oh
Copy link

w-oh commented Mar 4, 2024

Approved by UXD 3/4/24 (woh@)

@colifran colifran added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Mar 4, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review March 4, 2024 20:10

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

@aws-cdk-automation
Copy link
Collaborator

➡️ 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.

Signed-off-by: SankyRed <sankyred@amazon.com>
@vinayak-kukreja vinayak-kukreja removed the pr/do-not-merge This PR should not be merged at this time. label Mar 5, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

Copy link
Contributor

mergify bot commented Mar 5, 2024

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 a7fac9d into main Mar 5, 2024
14 of 15 checks passed
@mergify mergify bot deleted the vkukreja/list-stack-deps branch March 5, 2024 02:34
@SimonCMoore SimonCMoore mentioned this pull request Mar 27, 2024
2 tasks
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 pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants