-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Conversation
There was a problem hiding this 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.
```console | ||
$ # List all stacks in the CDK app 'node bin/main.js' |
There was a problem hiding this comment.
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.
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. |
69c8a9f
to
8f4b1ef
Compare
Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
8f4b1ef
to
43ce6f6
Compare
Exemption Request: We are cli integ testing this change which would not generate a snapshot. |
There was a problem hiding this 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.
packages/aws-cdk/lib/list-stacks.ts
Outdated
}); | ||
} | ||
} else { | ||
data.dependencies?.push({ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(...)
There was a problem hiding this comment.
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 && |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree :)
Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
Approved by UXD 3/4/24 (woh@) |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
Signed-off-by: SankyRed <sankyred@amazon.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Reason for this change
Existing
cdk list
functionality does not display stack dependencies. This PR introduces that functionality.For instance,
Existing functionality:
Feature functionality:
Description of changes
Changes are based on internal team design discussions.
--show-dependencies
is being introduced forlist
cli command.list-stacks.ts
is being added.listStacks
function within the file for listing stack and their dependencies using cloud assembly from the cdk toolkit.Description of how you validated changes
Checklist
Co-Author
Co-authored-by: @SankyRed
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license