Skip to content
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

add support for command type='container' #5821

Merged
merged 3 commits into from Feb 11, 2019

Conversation

Projects
4 participants
@danielcondemarin
Copy link
Contributor

danielcondemarin commented Feb 9, 2019

What did you implement:

Closes: #5806

How did you implement it:

  • Added a new command type "container", which can be used when you want nested commands but the top level command on its own doesn't do anything.
  • Note this is different from the entrypoint command type which hides its children commands.
  • "container" commands cannot be invoked, only its children.
  • "container" commands will not be displayed on the CLI, only its children.

How can we verify it:

ServerlessCustomPlugin.js

class ServerlessCustomPlugin {
  constructor(serverless, options) {
    this.serverless = serverless;
    this.options = options;

    this.commands = {
      mycommand: {
        type: 'container',
        commands: {
          foo: {
            usage: 'Child foo command',
            lifecycleEvents: ['foo1']
          },

          bar: {
            usage: 'Child bar command',
            lifecycleEvents: ['bar1']
          }
        }
      }
    };

    this.hooks = {
      'mycommand:foo:foo1': this.fooOne.bind(this),
      'mycommand:bar:bar1': this.barOne.bind(this)
    };
  }

  fooOne() {
    console.log('Hello foo1');
  }

  barOne() {
    console.log('Hello bar1');
  }
}

module.exports = ServerlessCustomPlugin;

As seen below, mycommand is not displayed on the CLI, only its children:

image

Also, when trying to invoke it directly fails:

image

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?: YES
Is it a breaking change?: NO

@pmuens pmuens added the pr/in-review label Feb 10, 2019

@dschep
Copy link
Member

dschep left a comment

Looks pretty good! Just a few clarifications please.

cli.displayCommandUsage(mycommand, 'mycommand');

expect(cli.consoleLog.calledOnce).to.equal(true);
expect(cli.consoleLog.calledWith(sinon.match('mycommand subcmd'))).to.equal(true);

This comment has been minimized.

@dschep

dschep Feb 11, 2019

Member

Shouldn't this be checking that mycommand(without subcommand) isn't printed in addition to mycommand cubcmd being printed?

This comment has been minimized.

@danielcondemarin

danielcondemarin Feb 11, 2019

Author Contributor

Is implicitly tested by making sure consoleLog is only called once (Ln 533). However I do agree adding that assertion would make it more explicit, so will add it

const commandExists = name in current.commands;
const isNotContainer = commandExists && current.commands[name].type !== 'container';
const isNotEntrypoint = commandExists && current.commands[name].type !== 'entrypoint';
const remainingIterationsLeft = index < commandOrAlias.length - 1;

This comment has been minimized.

@dschep

dschep Feb 11, 2019

Member

Could you explain the need for remainingIterationsLeft?

This comment has been minimized.

@danielcondemarin

danielcondemarin Feb 11, 2019

Author Contributor

If the current command is a container, and there are remaining iterations, as in, more subcommands in commandsArray to process, then it continues looping.

For example, suppose mycommand below is a container, by checking there are "remainingIterationsLeft" I make sure the reducer checks for subcmd too.

commandsArray: [ 'mycommand', 'subcmd' ]

On the other hand, if you have:

commandsArray: [ 'mycommand' ]

There wouldn't be any iterations left, and also because is a container command is going to error with sls command not found.

This comment has been minimized.

@dschep

dschep Feb 11, 2019

Member

Ah! That makes sense. Thanks.

@dschep dschep added this to In progress in Serverless via automation Feb 11, 2019

@dschep dschep self-assigned this Feb 11, 2019

Serverless automation moved this from In progress to Reviewer approved Feb 11, 2019

@dschep

dschep approved these changes Feb 11, 2019

const commandExists = name in current.commands;
const isNotContainer = commandExists && current.commands[name].type !== 'container';
const isNotEntrypoint = commandExists && current.commands[name].type !== 'entrypoint';
const remainingIterationsLeft = index < commandOrAlias.length - 1;

This comment has been minimized.

@dschep

dschep Feb 11, 2019

Member

Ah! That makes sense. Thanks.

@dschep dschep merged commit 7968df1 into serverless:master Feb 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Serverless automation moved this from Reviewer approved to Done Feb 11, 2019

@eahefnawy eahefnawy added this to the 1.38.0 milestone Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.