-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
refactor(CLI): Introduce modern warning about resource limit #10086
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10086 +/- ##
==========================================
+ Coverage 85.38% 85.41% +0.02%
==========================================
Files 333 334 +1
Lines 13571 13592 +21
==========================================
+ Hits 11588 11609 +21
Misses 1983 1983
Continue to review full report at Codecov.
|
lib/plugins/aws/info/index.js
Outdated
@@ -75,6 +76,8 @@ class AwsInfo { | |||
'finalize': () => { | |||
if (this.serverless.processedInput.commands.join(' ') !== 'info') return; | |||
|
|||
writeServiceResourceCountWarning(this.serverless.serviceResourceCount); |
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.
Maybe instead of creating extra util. We can simply host it here, but show it also in case of deploy
command ?
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 guess we can do that - in such case we would need to move it to be executed before the if
- does that sound good? I wanted to keep the approach of having it called explicitly in finalize
part of deploy
so it's easier to spot what would be printed
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 understand the concern, still creating a standalone util (which ideally should also be backed with tests) just to reuse warning log, doesn't feel right to me.
I think it's better to either write this log twice, or simply cover it just at info
, and I don't think it's harmful. Deploy specifically calls info logic, so it's clear that this logic may produce extra logs, and we cover printing service sections separately only because, output differs in case of both commands (otherwise I would also probably just keep it at infp)
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've changed the implementation following the suggestion and I'm not happy about the result - the problem here is that by still relying on finalize
from info
in deploy
, we lose control about ordering of display as finalize hook for deploy will be resolved first. See in screenshot for example:
Changing the order of finalize hooks also didn't solve the problem, as I think the warning should be displayed after the message that stack was deployed successfully and before the summary, and with approach of having implementation only in info
's finalize
hook that's just not possible. What do you think?
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.
Changing the order of finalize hooks also didn't solve the problem, as I think the warning should be displayed after the message that stack was deployed successfully and before the summary,
In that case maybe simply attach it unconditionally to after:aws:info:validate
hook?
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's a warning that's the outcome of a validation, so its handling should probably not belong to the logic point, where we summarize command outcome
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 that it's more of a warning that is outcome of validation - the problem with attaching it to validate hook does not solve ordering problem, as I think the warning should be displayed after "Service deployed to stack ....", which won't be the case anymore as that is printed in finalize
hook of deploy
, which will happen afterwards. I guess that would have to be the way it's done 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.
Why is this important to have this warning after Service deployed to stack ....
?
As I look into it, I don't think it'll be confusing to display it before, I'm actually a bit surprised we do not show it at the very beginning (right before deployment, and possibly even in sls package
command)
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.
Maybe it's not that important - my feeling was that it makes more sense to display it kind of as a part of summary information, after deployment.
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've looked into the history of this warning, and originally it was configured in this place, as it was part of the status table: #4822. So now we deal with the legacy of that initial idea,
Now it's a regular warning log, and all other warnings (with exception to deprecations) are usually shown at the earliest convenience.
I don't this warning log really should belong to the final status. I think it'll be even more valuable to show it before starting the deployment, and also in sls package
command (still I don't mean we should address this in this PR)
In this PR I will simply secure it as a regular warning, that shows before any final status message, and which is initiated from a single logic point, simply to not overcomplicate its processing
b035080
to
ca0e8cc
Compare
ca0e8cc
to
2d3a8a0
Compare
96d934f
to
d0958a3
Compare
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 👍
lib/plugins/aws/info/index.js
Outdated
@@ -75,6 +76,8 @@ class AwsInfo { | |||
'finalize': () => { | |||
if (this.serverless.processedInput.commands.join(' ') !== 'info') return; | |||
|
|||
writeServiceResourceCountWarning(this.serverless.serviceResourceCount); |
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've looked into the history of this warning, and originally it was configured in this place, as it was part of the status table: #4822. So now we deal with the legacy of that initial idea,
Now it's a regular warning log, and all other warnings (with exception to deprecations) are usually shown at the earliest convenience.
I don't this warning log really should belong to the final status. I think it'll be even more valuable to show it before starting the deployment, and also in sls package
command (still I don't mean we should address this in this PR)
In this PR I will simply secure it as a regular warning, that shows before any final status message, and which is initiated from a single logic point, simply to not overcomplicate its processing
@medikoo Could you clarify a little bit more about your suggestion - what would you change as a part of this PR because I'm not sure I fully understand it |
Oh I see, thanks for clarification 🙇 Let's wait for @mnapoli with final approval for the placement of this warning for now 👍 |
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.
Both placements, even if not consistent, are fine by me (I don't think it's critical both commands are identical).
Addresses: #9860
It looks like modern logs for this warning has been omitted previously, it brings it back to warn users that are approaching resource limit in CloudFormation