Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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): watch streams resources' CloudWatch logs to the terminal #18159
feat(cli): watch streams resources' CloudWatch logs to the terminal #18159
Changes from 34 commits
3add949
151b41d
d290fee
a13dafb
ad1b4a4
18fbefd
5bba356
94fc4c8
9e0bef8
809d3d8
cb078bf
2f1d9da
a0c8f21
2aa585d
8e0e475
5c2af13
beb9fc4
87afeab
0dc861d
4329b9c
ca38736
81e6634
d718fdb
b3ae63c
200fd60
bf6ef3f
c38de39
3f3547a
5043f4a
965f963
e46e971
4369e08
15f3e91
2c07e8f
87d8d9d
d875b92
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Any chance we can make this a method of
SdkProvider
?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 originally had it as a method of
SdkProvider
, but after discussing with @rix0rrr we decided to put it 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.
Yeah, the SdkProvider shouldn't know about CloudFormationStackArtifacts or the fact that SSM parameters hold bootstrap stack versions.
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: it's more like
CachedListStackResources
thanLazy*
.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.
Is there ever a chance we will list the stack resources before a particular resource is created, and then never update the list to account for the new resource?Aha I guess since we only use this for hotswapping, that will never add new resources so doesn't apply.
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.
In general, the PR description could do with a Cliff's Notes of the approach taken, biggest challenges, design decisions and refactorings. It's very helpful when evaluating a change.
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.
That is a good suggestion! This particular file was existing functionality that was moved here, which I could have called out.
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.
It is actually Lazy though (it will not make a service call unless you call
listResources()
).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.
That is true for most methods though 🤣
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.
Not sure what you mean. I think a very sensible non-lazy implementation would be to make the service call in the constructor of the class, and not in
listResources()
(it would be simpler, for one thing).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.
What is this function used for? To predict physical names, before deployment happens?
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.
Can't we just always list from the currently deployed 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 think the intent here was to avoid a lookup if we could ( cc @skinny85 )
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.
Yes, this is used in case we can get the physical name from the template, and in this way save a service call (not super relevant here, but very relevant for hotswapping, and this class is used in both places).