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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add resource count and warning to info display #4822

Merged
merged 8 commits into from Jan 24, 2019

Conversation

@alexdebrie
Copy link
Contributor

alexdebrie commented Mar 11, 2018

What did you implement:

This PR does two things:

  1. When displaying Stack Info, it includes a resources: indication that shows the number of resources in the CloudFormation stack:

resourcecount

  1. If there are 150+ resources in a service, it displays a warning that the user is approaching the 200 resource limit for CloudFormation and links them to a blog post on how to avoid it:
    resourcewarning

Hat tip to @Vadorequest for this idea initially. I thought it was a great idea. 馃挜 .

I'll update the tests as needed but want to make sure we're good on the visuals first. Once those are settled, I'll make sure the tests pass.

How did you implement it:

As part of the aws:info:gatherData hook, I make a listStackResources API call for the CloudFormation stack and add it to the gatheredData object. Then I print it out in the displayServiceInfo() method.

How can we verify it:

  1. Run sls info in a deployed service. It should show resources: in the info. Similarly, it should show in the Service Information block after a deploy.

  2. If you want to test the warning, set up a service with 30 HTTP functions and deploy it. It should display a warning.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: NO
Is it a breaking change?: NO

@Vadorequest

This comment has been minimized.

Copy link
Contributor

Vadorequest commented Mar 11, 2018

@alexdebrie Looks great on the visual/UI. As far as the implementation goes, it looks perfect. 馃憤

alexdebrie and others added some commits Mar 12, 2018

@horike37
Copy link
Member

horike37 left a comment

@alexdebrie
Nice! The information and warning would be beneficial for many users!
I just added some comments for the implementation.
Cloud you confirm those?

message += `${chalk.yellow('stack:')} ${info.stack}\n`;
message += `${chalk.yellow('resources:')} ${info.resourceCount}`;

if (info.resourceCount >= 150) {

This comment has been minimized.

@horike37

horike37 Mar 12, 2018

Member

Would be good to verify if it has the object key like this if (_.has(info, 'resourceCount') && info.resourceCount >= 150)

return this.provider.request('CloudFormation',
'listStackResources',
{ StackName: stackName })
.then((result) => {

This comment has been minimized.

@horike37

horike37 Mar 12, 2018

Member

You can write simpler like this .then(result => {

'listStackResources',
{ StackName: stackName })
.then((result) => {
if (result) {

This comment has been minimized.

@horike37

horike37 Mar 12, 2018

Member

Use lodash for consistency. if (!isEmpty(result)) {

outputs: [],
};

return awsInfo.getResourceCount().then(() => {

This comment has been minimized.

This comment has been minimized.

@alexdebrie

alexdebrie Mar 13, 2018

Author Contributor

@horike37 I don't quite understand this one. Which line is a good example in the plugin.test.js?

This comment has been minimized.

@horike37

horike37 Mar 13, 2018

Member

@alexdebrie
Please see this line. You can test if it returns fulfilled or rejected as a promise by using chai-as-promised. Otherwise, you can't handle the test process if the current test is and returns rejected.
https://github.com/serverless/serverless/blob/master/lib/plugins/plugin/plugin.test.js#L55

This comment has been minimized.

@alexdebrie

alexdebrie Mar 13, 2018

Author Contributor

@horike37 Thanks! I gave it a shot -- let me know if that's right 馃槃

outputs: [],
};

return awsInfo.getResourceCount().then(() => {

This comment has been minimized.

This comment has been minimized.

@alexdebrie

alexdebrie Mar 13, 2018

Author Contributor

@horike37 Same question as above

This comment has been minimized.

@horike37

horike37 Mar 13, 2018

Member

Ah, sorry. this is duplicate of the above comment, please ignore.

@horike37

This comment has been minimized.

Copy link
Member

horike37 commented Mar 13, 2018

@alexdebrie
I have re-thought about the warning, which may be annoying some people since the warning message is displayed when the number of resources is more than 150, and you can't remove that even if the problem has been resolved on users end.

I think the message may be unnecessary. What do you think?

@alexdebrie

This comment has been minimized.

Copy link
Contributor Author

alexdebrie commented Mar 13, 2018

@horike37 Yea, I see what you're saying. The problem is this can't ever really be "resolved" for a user unless they remove resources. If they have more than 150 resources, they're in that danger zone of hitting the 200 resource limit. This limit is a pretty common problem, so we'd like to give users a heads up before they run directly into it without warning.

Open to other thoughts on how to surface this for users while also not being annoying to others. cc @HyperBrain @brianneisler

@HyperBrain

This comment has been minimized.

Copy link
Member

HyperBrain commented Mar 13, 2018

@alexdebrie What about restricting the warning to verbose mode only?

this.options.verbose && this.serverless.cli.log(...);

Then it is hidden during normal installs and users can get the warning additionally to other detailed output. Maybe at some threshold it can be even emitted without verbose mode enabled, like something above 190, because then every new resource added is likely to break the deployment.

@alexdebrie

This comment has been minimized.

Copy link
Contributor Author

alexdebrie commented Mar 13, 2018

@HyperBrain To me, the biggest point of this is to provide helpful guidance to less-experienced users before they hit the wall head on. I'm guessing the less-experienced users won't be using verbose mode as often, and 190+ resources seems pretty late to start warning them.

How big of an annoyance is the additional warning message? I feel like it's a pretty small subset of users that:

  1. Have more than 150 resources;
  2. Understand the CloudFormation resource limit and are OK with their status; and
  3. Get annoyed by the extra output.

But maybe I'm misjudging that 馃槃

@HyperBrain

This comment has been minimized.

Copy link
Member

HyperBrain commented Mar 13, 2018

In regards to new users, it seems fair to emit the warning to let them know. I'm also fine to have the warning unconditionally for now 馃槃

A further improvement would then be to add log levels to the cli in general and put this "warning" into an "info" level. By default Serverless would show everything (error, warning and info) and thus show the warning for users that are new or unexperienced. More experienced users could then restrict the levels to "error" and/or "warning" by a config property or switch and would not see it.

I think with such a level implementation everyone can be happy.

@alexdebrie

This comment has been minimized.

Copy link
Contributor Author

alexdebrie commented Mar 13, 2018

@HyperBrain Agreed! That would be a good solution 馃槃

@Vadorequest

This comment has been minimized.

Copy link
Contributor

Vadorequest commented Mar 13, 2018

My first thought about it was to allow to add some kind of settings in the serverless.yml in order to hide such messages, that way people would get the warning and if it gets annoying they can just disable it through the serverless.yml file. But logging level works too, as long as it's displayed by default because that's too sensitive to be hidden by default.

@HyperBrain

This comment has been minimized.

Copy link
Member

HyperBrain commented Mar 13, 2018

Logging levels are imo better, because settings in serverless.yml are service configuration settings. The output of warning messages is more a Serverless CLI setting than a service/project setting.
So, having log level settings in the SLS configuration file (I think it was serverlessrc globally or locally) would be the proper approach.

@HyperBrain

This comment has been minimized.

Copy link
Member

HyperBrain commented Mar 13, 2018

I restarted the Travis build (had a hang). However, I'm not allowed to restart the appveyor build 馃 . @pmuens Can you give me some rights there?

@Vadorequest

This comment has been minimized.

Copy link
Contributor

Vadorequest commented Mar 13, 2018

@HyperBrain I actually agree, it has nothing to do with the serverless.yml indeed. It was just my first thought, but logging level makes more sense.

@ronkorving

This comment has been minimized.

Copy link
Contributor

ronkorving commented Jul 13, 2018

This is a great feature! I'm sad to see this hasn't gotten much attention for a while. Is this still going places?

@pmuens
Copy link
Member

pmuens left a comment

This looks great! Thanks @alexdebrie 馃憤

Nice feature which can be really helpful! I just looked through the code and it's almost GTM. Just added one comment regarding a removed restoring of a stub. Other than that I think it should be GTM :shipit:

@@ -52,7 +55,7 @@ describe('AwsInfo', () => {

afterEach(() => {
awsInfo.validate.restore();
awsInfo.getStackInfo.restore();

This comment has been minimized.

@pmuens

pmuens Jan 23, 2019

Member

What happened to getStackInfo here? Shouldn't we restore it as well since I see the corresponding stub above 馃

This comment has been minimized.

@alexdebrie

alexdebrie Jan 23, 2019

Author Contributor

Good catch, @pmuens ! I have no idea why I did that, so I added it back. 馃槃

@pmuens pmuens self-assigned this Jan 23, 2019

@pmuens pmuens added pr/in-review and removed pr/in-progress labels Jan 23, 2019

@pmuens pmuens added this to In progress in Serverless via automation Jan 23, 2019

@eahefnawy
Copy link
Member

eahefnawy left a comment

A very belated "thank you" @alexdebrie 馃槃

Serverless automation moved this from In progress to Needs review Jan 23, 2019

@pmuens

pmuens approved these changes Jan 24, 2019

Copy link
Member

pmuens left a comment

Thanks for updating @alexdebrie 馃憤

And +100 for the descriptive commit message "Restore the missing restore " 馃憣 馃槀

This feature is super nifty. Just gave it a spin and it works great! :shipit:

Stale review

Serverless automation moved this from Needs review to Reviewer approved Jan 24, 2019

@pmuens pmuens merged commit 36f9829 into master Jan 24, 2019

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+4.2%) to 93.011%
Details

Serverless automation moved this from Reviewer approved to Done Jan 24, 2019

@pmuens pmuens deleted the ResourceCount branch Jan 24, 2019

@pmuens pmuens added this to the 1.36.4 milestone Feb 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment