Support `template-path` and `name` CLI options for interactive CLI #9471

pgrzesik · 2021-05-13T08:57:20Z

Addresses: #9367

pgrzesik · 2021-05-13T09:00:05Z

lib/cli/interactive-setup/service.js

+    if (options['template-path']) {
+      projectName = await resolveProjectNameInput(options, workingDir);
+      projectDir = join(workingDir, projectName);
+      createFromLocalTemplate({


I've went with the approach of throwing errors in cases where the invalid options were supplied (name and template-path), but I'm not 100% sure about this approach of presenting errors to users

IMO good call, I think we should validate CLI options with real thrown errors, as those are forced settings (we're not re-asking user to pass fixed CLI option), and via CLI options we also want to support non interactive usage, and in such scenario it's very important that processes exit with non zero code.

codecov · 2021-05-13T09:01:47Z

Codecov Report

Merging #9471 (f876719) into master (105807a) will increase coverage by 0.03%.
The diff coverage is 97.87%.

@@            Coverage Diff             @@
##           master    #9471      +/-   ##
==========================================
+ Coverage   86.77%   86.80%   +0.03%     
==========================================
  Files         327      328       +1     
  Lines       11985    12022      +37     
==========================================
+ Hits        10400    10436      +36     
- Misses       1585     1586       +1

Impacted Files	Coverage Δ
lib/cli/commands-schema/no-service.js	`100.00% <ø> (ø)`
lib/utils/create-from-local-template.js	`93.33% <93.33%> (ø)`
lib/cli/interactive-setup/service.js	`100.00% <100.00%> (ø)`
lib/Serverless.js	`91.51% <0.00%> (ø)`
lib/cli/handle-error.js	`88.75% <0.00%> (ø)`

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 105807a...f876719. Read the comment docs.

medikoo

@pgrzesik looks great ! I have just few minor suggestions, and we should be good to go

medikoo · 2021-05-13T09:37:38Z

lib/cli/interactive-setup/service.js

 module.exports = {
  isApplicable({ serviceDir }) {
    return !serviceDir;
  },
  async run(context) {
    const workingDir = context.cwd || process.cwd();
+    const options = context.options || {};


Options are assumed to be provided unconditionally here

medikoo · 2021-05-13T09:41:12Z

lib/utils/create-from-local-template.js

+  const sourcePath = untildify(templatePath);
+
+  try {
+    copyDirContentsSync(sourcePath, projectDir, { noLinks: true });


Let's use fse.copy instead (we also consider *Sync functions as deprecated, and they're scheduled to be replaced, and such utils removed -> #8604)

medikoo · 2021-05-13T09:42:31Z

lib/utils/create-from-local-template.js

+        'INVALID_TEMPLATE_PATH'
+      );
+    }
+    throw err;


I this case I'd also throw ServerlessError, with message as Cannot copy template path: ${error.message}

medikoo · 2021-05-13T09:45:15Z

lib/cli/interactive-setup/service.js

 module.exports = {
  isApplicable({ serviceDir }) {
    return !serviceDir;
  },
  async run(context) {
    const workingDir = context.cwd || process.cwd();
+    const options = context.options || {};
    const isConfirmed = await confirm('No project detected. Do you want to create a new one?', {


I think this question should be skipped if any of those option is passed (it's a clear indication, that intention is to create a new project)

medikoo · 2021-05-13T09:56:12Z

lib/cli/interactive-setup/service.js

+    if (options['template-path']) {
+      projectName = await resolveProjectNameInput(options, workingDir);
+      projectDir = join(workingDir, projectName);
+      createFromLocalTemplate({


IMO good call, I think we should validate CLI options with real thrown errors, as those are forced settings (we're not re-asking user to pass fixed CLI option), and via CLI options we also want to support non interactive usage, and in such scenario it's very important that processes exit with non zero code.

medikoo · 2021-05-13T09:57:32Z

lib/cli/interactive-setup/service.js

+
+  return projectNameInput(workingDir);
+};
+
 module.exports = {
  isApplicable({ serviceDir }) {
    return !serviceDir;


I think we should also throw here errors, if given options were passed but user is in service context (we should not silently ignore them)

pgrzesik · 2021-05-13T10:05:30Z

Thanks for review @medikoo - thanks for raising the point about serviceDir as I wanted to bring this topic up as well - I think that in cases where these options are provided, we should no longer consider it to be a fully interactive setup that detects if a project exists in current directory but rather already assume that we're going to create a new project vs "enhancing" the existing one. I'll make the adjustments 👍

medikoo

Looks great 👍 I've just suggested changes to error messaging, let me know what you think

medikoo · 2021-05-13T12:19:41Z

lib/cli/interactive-setup/service.js

+  isApplicable({ options, serviceDir }) {
+    if (serviceDir && (options.name || options['template-path'])) {
+      throw new ServerlessError(
+        'Project already exists in current directory. If you want to continue its setup, run "serverless" without `--name`, `--template-path` options. Otherwise, if you want to setup a new project, run the command in a non-service directory.',


I'd simplify the message to:

Cannot setup a new service being in context of the other service ("--name" and "--template-path" options cannot be applied)

What do you think?

medikoo · 2021-05-13T12:22:08Z

lib/cli/interactive-setup/service.js

+    if (serviceDir && (options.name || options['template-path'])) {
+      throw new ServerlessError(
+        'Project already exists in current directory. If you want to continue its setup, run "serverless" without `--name`, `--template-path` options. Otherwise, if you want to setup a new project, run the command in a non-service directory.',
+        'INTERACTIVE_SERVICE_ALREADY_EXISTS'


We do not do INTERACTIVE_ prefix in case of other errors here, so I would probably not set it here.

Maybe let's name it simply NOT_APPLICABLE_SERVICE_OPTIONS (?) (when processing errors we have information of command being used, so that doesn't have to leak to error codes)

medikoo

Looks great 👍

pgrzesik added 2 commits May 13, 2021 10:15

refactor: Add create-from-local-template util

fcf22e9

feat(CLI Onboarding): Support template-path param

1ccc4d2

pgrzesik added cat/cli-onboarding Interactive CLI onboarding enhancement labels May 13, 2021

pgrzesik force-pushed the support-template-path-and-path-in-interactive-cli branch from 5c25c7a to 2b7ba82 Compare May 13, 2021 08:57

pgrzesik commented May 13, 2021

View reviewed changes

pgrzesik requested a review from medikoo May 13, 2021 09:09

medikoo suggested changes May 13, 2021

View reviewed changes

medikoo assigned pgrzesik May 13, 2021

pgrzesik added 2 commits May 13, 2021 12:52

feat(CLI Onboarding): Support --name CLI option

cc3fefb

feat(CLI Onboarding): Skip "enable" question when options provided

e602fa2

pgrzesik force-pushed the support-template-path-and-path-in-interactive-cli branch 2 times, most recently from 8d386a8 to af0007e Compare May 13, 2021 11:13

pgrzesik requested a review from medikoo May 13, 2021 12:04

medikoo reviewed May 13, 2021

View reviewed changes

feat(CLI Onboarding): Dont allow setup with options in service dir

f876719

pgrzesik force-pushed the support-template-path-and-path-in-interactive-cli branch from af0007e to f876719 Compare May 13, 2021 12:33

pgrzesik requested a review from medikoo May 13, 2021 12:38

medikoo approved these changes May 14, 2021

View reviewed changes

pgrzesik merged commit 7e8e1b6 into master May 14, 2021

pgrzesik deleted the support-template-path-and-path-in-interactive-cli branch May 14, 2021 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `template-path` and `name` CLI options for interactive CLI #9471

Support `template-path` and `name` CLI options for interactive CLI #9471

pgrzesik commented May 13, 2021

pgrzesik May 13, 2021 •

edited

medikoo May 13, 2021

codecov bot commented May 13, 2021 •

edited

medikoo left a comment

medikoo May 13, 2021

medikoo May 13, 2021

medikoo May 13, 2021

medikoo May 13, 2021

medikoo May 13, 2021

medikoo May 13, 2021

pgrzesik commented May 13, 2021

medikoo left a comment

medikoo May 13, 2021

medikoo May 13, 2021

medikoo left a comment

Support template-path and name CLI options for interactive CLI #9471

Support template-path and name CLI options for interactive CLI #9471

Conversation

pgrzesik commented May 13, 2021

pgrzesik May 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 13, 2021 • edited

Codecov Report

medikoo left a 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

pgrzesik commented May 13, 2021

medikoo left a 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

Support `template-path` and `name` CLI options for interactive CLI #9471

Support `template-path` and `name` CLI options for interactive CLI #9471

pgrzesik May 13, 2021 •

edited

codecov bot commented May 13, 2021 •

edited