Remove help verbose 8497 #8532

vinod-tahelyani · 2020-11-21T17:39:04Z

Closes: #8497

The new output looks something like-

Commands

You can run commands with "serverless" or the shortcut "sls"
Pass "--no-color" to disable CLI colors
Pass "--help" after any for contextual help

Interactive Quickstart

Run serverless (or shortcut sls) without any arguments to initialize an interactive setup
of functionalities related to given service or current environment
Pass "--help-interactive" for contextual help on interactive CLI options

Serverless Components

Run serverless (or shortcut sls) in context of a component service to initialize a components CLI
Pass "--help-components" for contextual help on Serverless Components

Framework

Documentation: http://slss.io/docs

Environment Variables

Set SLS_DEBUG=* to see debugging logs
Set SLS_WARNING_DISABLE=* to hide warnings from the output
Set SLS_MAX_CONCURRENT_ARTIFACTS_UPLOADS to control the maximum S3 upload SDK requests that are sent in parallel during the deployment of the service's artifacts. The default is 3. Note: increasing this too high might, actually, downgrade the overall upload speed

General Commands

config ........................ Configure Serverless
config credentials ............ Configures a new provider profile for the Serverless Framework
config tabcompletion install .. Install a completion for chosen shell
config tabcompletion uninstall Uninstall a completion for chosen shell
create ........................ Create new Serverless service
dashboard ..................... Open the Serverless dashboard
deploy ........................ Deploy a Serverless service
deploy function ............... Deploy a single function from the service
deploy list ................... List deployed version of your Serverless Service
deploy list functions ......... List all the deployed functions and their versions
generate-event ................ Generate event
info .......................... Display information about the service
install ....................... Install a Serverless service from GitHub or a plugin from the Serverless registry
invoke ........................ Invoke a deployed function
invoke local .................. Invoke function locally
login ......................... Login or sign up for Serverless
logout ........................ Logout from Serverless
logs .......................... Output the logs of a deployed function
metrics ....................... Show metrics for a specific function
output ........................
output get .................... Get value of dashboard deployment profile parameter
output list ................... List all dashboard deployment profile parameters
package ....................... Packages a Serverless service
param .........................
param get ..................... Get value of dashboard service output
param list .................... List all dashboard service outputs
plugin ........................ Plugin management for Serverless
plugin install ................ Install and add a plugin to your service
plugin uninstall .............. Uninstall and remove a plugin from your service
plugin list ................... Lists all available plugins
plugin search ................. Search for plugins
print ......................... Print your compiled and resolved config file
remove ........................ Remove Serverless service and all resources
rollback ...................... Rollback the Serverless service to a specific deployment
rollback function ............. Rollback the function to a specific version
ses-template .................. Manage AWS SES templates
ses-template deploy ........... Sync email templates to AWS SES
ses-template delete ........... Delete email template from AWS SES by given name
ses-template list ............. List email templates from AWS SES
slstats ....................... Enable or disable stats
studio ........................ Develop a Serverless application in the cloud.
test .......................... Run HTTP tests

Plugins
DynamoDBAutoscaling, ServerlessSesTemplate

Commands by plugin

ServerlessSesTemplate
ses-template .................. Manage AWS SES templates
ses-template deploy ........... Sync email templates to AWS SES
ses-template delete ........... Delete email template from AWS SES by given name
ses-template list ............. List email templates from AWS SES

…r.loadCommands() Added isExternal feild to plugins and commands Passing isExternal: true for all plugins resolved via resolveServicePlugins() method pluginManager.loadAllPlugins()

Changed logging for help, now help will output all the OOTB commands in a single list, while commands form external pluging will be listed as group by plugin Plugins section in help will list only external plugin

codecov-io · 2020-11-21T17:56:55Z

Codecov Report

Merging #8532 (14a5865) into master (85e480b) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8532      +/-   ##
==========================================
+ Coverage   87.24%   87.26%   +0.01%     
==========================================
  Files         255      255              
  Lines        9503     9509       +6     
==========================================
+ Hits         8291     8298       +7     
+ Misses       1212     1211       -1

Impacted Files	Coverage Δ
lib/classes/CLI.js	`93.46% <100.00%> (+0.09%)`	⬆️
lib/classes/PluginManager.js	`96.62% <100.00%> (+0.05%)`	⬆️
lib/configSchema.js	`100.00% <0.00%> (ø)`
lib/plugins/aws/provider/awsProvider.js	`93.06% <0.00%> (ø)`
...lugins/aws/package/compile/events/httpApi/index.js	`92.59% <0.00%> (ø)`
lib/classes/Service.js	`86.17% <0.00%> (+0.57%)`	⬆️

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 85e480b...14a5865. Read the comment docs.

vinod-tahelyani · 2020-11-21T18:07:29Z

@medikoo pls review this

medikoo

Thank you @vinod-tahelyani ! It looks very good. I've proposed few improvements. Let me know what you think

medikoo · 2020-11-23T08:46:00Z

lib/classes/PluginManager.js

  }

-  addPlugin(Plugin) {
+  addPlugin(Plugin, isExternal = false) {


Let's use options object ({ isExternal } instead of isExternal).

This makes a better API (see: https://ariya.io/2011/08/hall-of-api-shame-boolean-trap)

medikoo · 2020-11-23T08:47:01Z

lib/classes/PluginManager.js

  }

-  loadCommands(pluginInstance) {
+  loadCommands(pluginInstance, isExternal) {


medikoo · 2020-11-23T08:47:41Z

lib/classes/PluginManager.js

          return event;
        });
        this.commands[key] = _.merge({}, this.commands[key], command);
+        Object.assign(this.commands[key], { isExternal });


We can add it to above _.merge call, no need for extra Object.assign

lib/classes/CLI.js

medikoo · 2020-11-23T09:03:06Z

lib/classes/PluginManager.js

+  addPlugin(Plugin, isExternal = false) {
    const pluginInstance = new Plugin(this.serverless, this.cliOptions);
+    // isExternal differentiates plugin as OOTB or not
+    Object.assign(pluginInstance, { isExternal });


It'll be more transparent to create this.externalPlugins collection (as set instance), and just group all the plugins there.

Assigning a property to a plugin instance, while should in most cases be harmless, it's destructive, note that we're interferring into plugin API, we can't be sure if we won't break things

u mean keeping this.plugins as it is and adding this.externalPlugins, right ?

Yes, exactly

medikoo · 2020-11-23T09:04:15Z

lib/classes/CLI.js

+      this.consoleLog(externalSortedPlugins.map(plugin => plugin.constructor.name).join(', '));
+
+      this.consoleLog('');
+      this.consoleLog(chalk.yellow.underline('Commands by plugin'));


Let's show this section, only if we have any commands added by plugins

medikoo · 2020-11-23T09:05:42Z

lib/classes/CLI.js

+        this.consoleLog('');
+      });
    } else {
      this.consoleLog('No plugins added yet');


I assume this is to be displayed, when no external plugins are configured (?)

I think we can skip this message (In current implementation it was never shown, as there were always some internal plugins)

but in this case under 'Plugin', we do not list internal plugins, So this can be seen right ?

Yes, and we don't want to be seen. Message is weird "No plugins added yet" (why yet?).

I think best way is to not show "Plugins" section at all

…n CLI.js

vinod-tahelyani · 2020-11-23T19:09:48Z

@medikoo done with all the changes, pls tell me what do u think

medikoo

Great thanks @vinod-tahelyani! It looks way better, we're close, Please see my final suggestions

medikoo · 2020-11-24T11:25:31Z

lib/classes/CLI.js

    this.serverless = serverless;
    this.inputArray = inputArray || null;
    this.loadedPlugins = [];
+    this.loadedExternalPlugins = new Set();


Let's not introduce this collection (we can simply access the external plugins from plugin manager via this.serverless.pluginManager.externalPlugins)

I understand that you've tried to maintain the existing convention, however some of the conventions as found in the code-base are old, unnecessarily verbose (imply maintenance cost, which could be avoided), and we try to not follow them with new code.

medikoo · 2020-11-24T11:25:43Z

lib/classes/CLI.js

    this.loadedPlugins = plugins;
  }

+  setLoadedExternalPlugins(externalPlugins) {


Let's not define this method

medikoo · 2020-11-24T11:28:04Z

lib/classes/CLI.js

    if (Object.keys(this.loadedCommands).length) {
-      Object.entries(this.loadedCommands).forEach(([command, details]) => {
+      const internalCommands = Object.entries(this.loadedCommands).filter(
+        command => command && !command.isExternal


I think this doesn't filter as expected, as in Object.entries result, each item is a [key, value] array, so command here is an array.

Simple fix would be to use Object.values instead, which is also more appriopriate in this case

medikoo · 2020-11-24T11:30:23Z

lib/classes/CLI.js

+    this.consoleLog('');
+    this.consoleLog(chalk.yellow.underline('General Commands'));
    this.consoleLog('');
    if (Object.keys(this.loadedCommands).length) {


Let's remove this if clause (in all cases there'll be some commands)

medikoo · 2020-11-24T11:31:34Z

lib/classes/CLI.js


-    // print all the installed plugins
-    this.consoleLog(chalk.yellow.underline('Plugins'));
+    const externalSortedPlugins = this.loadedPlugins


We can just name it externalPlugins (externalSortedPlugins would make sense if we would also host unsorted collection, but here it's not the case)

medikoo · 2020-11-24T11:34:11Z

lib/classes/PluginManager.js

+    const internalPluginOptions = { isExternal: false };
+    const externalPluginOptions = { isExternal: true };


Let's not create this variables, we can simply hardcode { isExternal: true } as second argument when adding external plugins and in other cases do not pass any options at all (as options are optional)

medikoo · 2020-11-24T11:34:35Z

lib/classes/PluginManager.js

  }

-  loadCommands(pluginInstance) {
+  loadCommands(pluginInstance, options) {


Let's mark options as optional argument, by assigning a default value

medikoo · 2020-11-24T11:35:46Z

lib/classes/PluginManager.js

          return event;
        });
-        this.commands[key] = _.merge({}, this.commands[key], command);
+        this.commands[key] = _.merge({}, this.commands[key], command, options);


It'll be more future proof, to specifically pass { isExternal: options.isExternal } (by design options are loadCommands operation options, and not command extension, and here we treat it as if it's command extension)

medikoo · 2020-11-24T11:36:01Z

lib/classes/PluginManager.js

    return this.plugins;
  }

+  getExternalPlugins() {


No need for that method

Removed unnecessary constants

Renamed externalSortedPlugins -> externalPlugins

Passing { isExternal: options.isExternal } in _.merge in line 289

vinod-tahelyani · 2020-11-24T18:16:09Z

@medikoo done

medikoo

Thank you @vinod-tahelyani ! That looks great

Vinod Tahelyani added 4 commits November 21, 2020 22:27

Added isExternal option in pluginManager.addPlugin() and pluginManage…

c0ec96a

…r.loadCommands() Added isExternal feild to plugins and commands Passing isExternal: true for all plugins resolved via resolveServicePlugins() method pluginManager.loadAllPlugins()

Removed --verbose option

6d9785f

Changed logging for help, now help will output all the OOTB commands in a single list, while commands form external pluging will be listed as group by plugin Plugins section in help will list only external plugin

Removed CLI.generateVerboseHelp()

784701a

Removed cli verbose test

bb788b6

medikoo suggested changes Nov 23, 2020

View reviewed changes

medikoo assigned vinod-tahelyani Nov 23, 2020

medikoo added cat/cli refactor enhancement and removed refactor labels Nov 23, 2020

Vinod Tahelyani added 7 commits November 23, 2020 21:10

Changed arg isExternal to option

3533a09

Added externalPlugins in PluginManager.js and loadedExternalPlugins i…

da78e3b

…n CLI.js

Removed 'No Plugins added yet'

d85c108

Correcting check for pluginCommands

ea48fc8

Correcting typo error- loadedExternalPlugins

e230845

Added default option for addPlugin

f5890f5

Merging options and commands together

2078ae9

medikoo suggested changes Nov 24, 2020

View reviewed changes

Vinod Tahelyani added 4 commits November 24, 2020 21:21

Removed loadedExternalPlugins from CLI.js

cb95678

Added default options value to PluginManager.loadCommands()

fcef419

Removed unnecessary constants

Removed feild CLI.loadedExternalPlugins

42bcbf5

Renamed externalSortedPlugins -> externalPlugins

Removed PluginManager.getExternalPlugins()

14a5865

Passing { isExternal: options.isExternal } in _.merge in line 289

medikoo approved these changes Nov 25, 2020

View reviewed changes

medikoo merged commit 4287494 into serverless:master Nov 25, 2020

medikoo mentioned this pull request Nov 25, 2020

Seclude CLI params and service config resolution from the core #8364

Closed

20 tasks

medikoo mentioned this pull request May 6, 2021

CLI: Do not validate command when falling back to old version #9437

Merged

		const internalPluginOptions = { isExternal: false };
		const externalPluginOptions = { isExternal: true };

Remove help verbose 8497 #8532

Remove help verbose 8497 #8532

Uh oh!

Conversation

vinod-tahelyani commented Nov 21, 2020

The new output looks something like-

Uh oh!

codecov-io commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vinod-tahelyani commented Nov 21, 2020

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinod-tahelyani commented Nov 23, 2020

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinod-tahelyani commented Nov 24, 2020

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented Nov 21, 2020 •

edited

Loading