refactor(CLI): Introduce modern warning about resource limit #10086

pgrzesik · 2021-10-12T13:30:13Z

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

codecov · 2021-10-12T13:35:19Z

Codecov Report

Merging #10086 (d0958a3) into master (db85602) will increase coverage by 0.02%.
The diff coverage is 66.66%.

@@            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

Impacted Files	Coverage Δ
lib/plugins/aws/info/index.js	`97.22% <66.66%> (-2.78%)`	⬇️
lib/plugins/aws/remove/lib/bucket.js	`100.00% <0.00%> (ø)`
...gins/aws/package/compile/events/s3/configSchema.js	`100.00% <0.00%> (ø)`
lib/utils/aws-schema-get-cf-value.js	`100.00% <0.00%> (ø)`
...ib/plugins/aws/package/lib/generateCoreTemplate.js	`95.55% <0.00%> (+0.20%)`	⬆️
lib/plugins/aws/provider.js	`94.27% <0.00%> (+0.30%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db85602...d0958a3. Read the comment docs.

medikoo · 2021-10-12T14:11:37Z

lib/plugins/aws/info/index.js

@@ -75,6 +76,8 @@ class AwsInfo {
      'finalize': () => {
        if (this.serverless.processedInput.commands.join(' ') !== 'info') return;

+        writeServiceResourceCountWarning(this.serverless.serviceResourceCount);


Maybe instead of creating extra util. We can simply host it here, but show it also in case of deploy command ?

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

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)

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?

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?

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

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

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)

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.

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

lib/cli/write-service-resource-count-warning.js

medikoo

Looks great 👍

medikoo · 2021-10-13T09:26:08Z

lib/plugins/aws/info/index.js

@@ -75,6 +76,8 @@ class AwsInfo {
      'finalize': () => {
        if (this.serverless.processedInput.commands.join(' ') !== 'info') return;

+        writeServiceResourceCountWarning(this.serverless.serviceResourceCount);


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

pgrzesik · 2021-10-13T14:23:12Z

@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

medikoo · 2021-10-13T14:28:33Z

@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

@pgrzesik it's all good, I've already approved it. What sneaked in is an old comment which didn't land earlier because of GitHub outage

pgrzesik · 2021-10-13T14:30:58Z

@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

@pgrzesik it's all good, I've already approved it. What sneaked in is an old comment which didn't land earlier because of GitHub outage

Oh I see, thanks for clarification 🙇 Let's wait for @mnapoli with final approval for the placement of this warning for now 👍

mnapoli

Both placements, even if not consistent, are fine by me (I don't think it's critical both commands are identical).

pgrzesik added refactor cat/cli labels Oct 12, 2021

pgrzesik self-assigned this Oct 12, 2021

pgrzesik requested review from medikoo and mnapoli October 12, 2021 13:39

medikoo reviewed Oct 12, 2021

View reviewed changes

mnapoli reviewed Oct 12, 2021

View reviewed changes

lib/cli/write-service-resource-count-warning.js Outdated Show resolved Hide resolved

pgrzesik force-pushed the display-warning-about-resource-limit branch from b035080 to ca0e8cc Compare October 13, 2021 09:19

refactor(CLI): Introduce modern warning about resource limit

2d3a8a0

pgrzesik force-pushed the display-warning-about-resource-limit branch from ca0e8cc to 2d3a8a0 Compare October 13, 2021 09:20

refactor(CLI): Remove empty line in info command

d0958a3

pgrzesik force-pushed the display-warning-about-resource-limit branch from 96d934f to d0958a3 Compare October 13, 2021 14:04

pgrzesik requested review from medikoo and mnapoli October 13, 2021 14:12

medikoo approved these changes Oct 13, 2021

View reviewed changes

mnapoli approved these changes Oct 13, 2021

View reviewed changes

pgrzesik merged commit 03b4b3d into master Oct 13, 2021

pgrzesik deleted the display-warning-about-resource-limit branch October 13, 2021 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(CLI): Introduce modern warning about resource limit #10086

refactor(CLI): Introduce modern warning about resource limit #10086

pgrzesik commented Oct 12, 2021

codecov bot commented Oct 12, 2021 •

edited

medikoo Oct 12, 2021

pgrzesik Oct 12, 2021

medikoo Oct 12, 2021

pgrzesik Oct 13, 2021

medikoo Oct 13, 2021

medikoo Oct 13, 2021

pgrzesik Oct 13, 2021

medikoo Oct 13, 2021

pgrzesik Oct 13, 2021

medikoo Oct 13, 2021

medikoo left a comment

medikoo Oct 13, 2021

pgrzesik commented Oct 13, 2021

medikoo commented Oct 13, 2021

pgrzesik commented Oct 13, 2021

mnapoli left a comment

refactor(CLI): Introduce modern warning about resource limit #10086

refactor(CLI): Introduce modern warning about resource limit #10086

Conversation

pgrzesik commented Oct 12, 2021

codecov bot commented Oct 12, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrzesik commented Oct 13, 2021

medikoo commented Oct 13, 2021

pgrzesik commented Oct 13, 2021

mnapoli left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 12, 2021 •

edited