feat(CLI Onboarding): Add deploy step #9536

pgrzesik · 2021-06-01T09:43:17Z

Addresses: #9367

medikoo · 2021-06-01T10:23:12Z

lib/Serverless.js

@@ -32,6 +32,7 @@ class Serverless {
    let configObject = config;
    configObject = configObject || {};
    this._isInvokedByGlobalInstallation = Boolean(configObject._isInvokedByGlobalInstallation);
+    this.isInvokedByInteractive = Boolean(configObject.isInvokedByInteractive);


I think we can avoid any internals hacking, by injecting the plugin as:

const serverless = new Serverless(...); await serverless.init(); serverless.pluginManager.addPlugin(InteractiveDeployPluginConstructor);

Have you tried that?

Yes, changed to use this approach

medikoo · 2021-06-01T10:24:11Z

lib/classes/CLI.js

@@ -44,6 +44,12 @@ class CLI {
    this.log = function () {};
  }

+  suppressLog() {


What exactly was the problem with overriding the process.stdout via process-utils/override-stdout?

Ideally we should not patch internals to achieve that

Changed the approach to process-utils/override-stdout

Ok, so I believe we no longer need this extension (?)

Correct 👍

lib/cli/interactive-setup/deploy.js

lib/plugins/deploy-interactive.js

pgrzesik · 2021-06-02T14:25:50Z

@medikoo - I've changed the approach to patching stdout as you suggested. Could you take a look to confirm the direction is correct? If that's the case, I'll complete the PR with tests + full impl of isApplicable. As for that, I have a question about the approach to take. I'm a bit torn as we have two potential approaches:

Fetch providers again and see if the service has an explicit provider or if there's a default one
Expand history in context to track that provider was succesfully setup as a part of aws-credentials step

No 1. has the advantage of not depending on previous step, while no 2 saves some extra complexity/API calls to backend. As we only want to consider this deploy step for new services, I'm leaning towards second approach, but I'd love to hear your opinion on that.

medikoo

Fetch providers again and see if the service has an explicit provider or if there's a default one

You've raised a good point, we didn't explore so far.

I think we simply need to ensure that AWS creds are loaded into environment when service is configured for dashboard, user is logged in to dashboard and there are provider credentials provided for service.

So I'd add following:

At service step, if user had setup a dashboard ready service with org pre-configured (users may use custom templates), ensure creds are loaded from providers
At dashboard-set-org step ensure to load creds if for given org & service, creds are accessible
At aws-credentials step, if user created a provider, ensure to automatically load those creds into process

Having above, we should probably double confirm prior deploy that AWS credentials are loaded, and follow with deploy if we detect they're

medikoo · 2021-06-05T11:30:09Z

lib/classes/CLI.js

@@ -44,6 +44,12 @@ class CLI {
    this.log = function () {};
  }

+  suppressLog() {


Ok, so I believe we no longer need this extension (?)

lib/cli/interactive-setup/aws-credentials.js

codecov · 2021-06-09T09:26:01Z

Codecov Report

Merging #9536 (97d9585) into master (feb0421) will decrease coverage by 0.02%.
The diff coverage is 81.92%.

❗ Current head 97d9585 differs from pull request most recent head 2768e9f. Consider uploading reports for the commit 2768e9f to get more accurate results

@@            Coverage Diff             @@
##           master    #9536      +/-   ##
==========================================
- Coverage   86.14%   86.11%   -0.03%     
==========================================
  Files         324      326       +2     
  Lines       12331    12390      +59     
==========================================
+ Hits        10622    10670      +48     
- Misses       1709     1720      +11

Impacted Files	Coverage Δ
lib/cli/interactive-setup/index.js	`100.00% <ø> (ø)`
...ib/cli/interactive-setup/deploy-progress-plugin.js	`21.42% <21.42%> (ø)`
lib/cli/interactive-setup/utils.js	`93.75% <91.66%> (-6.25%)`	⬇️
lib/cli/interactive-setup/deploy.js	`93.75% <93.75%> (ø)`
lib/cli/interactive-setup/aws-credentials.js	`88.88% <100.00%> (+1.25%)`	⬆️
lib/cli/interactive-setup/service.js	`100.00% <100.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 feb0421...2768e9f. Read the comment docs.

pgrzesik · 2021-06-09T09:39:38Z

@medikoo This is currently based on the PR with provider flow implementation to exclude changes, that's why there are no test runs reported, it will be run again after changing the base branch

medikoo

@pgrzesik looks great! I have just few, mostly organization-wise concerns

medikoo · 2021-06-09T09:45:14Z

lib/cli/interactive-setup/deploy.js

+      process.stdout.write(
+        `\n${chalk.green('Your project is live and available in ')}${chalk.white.bold(
+          `./${serviceName}`
+        )}\n`
+      );
+      process.stdout.write(`\n  Run ${chalk.bold('serverless info')} in the project directory\n`);
+      process.stdout.write('    View your endpoints and services\n');
+      process.stdout.write(`\n  Open ${chalk.bold(getDashboardInteractUrl(dashboardPlugin))}\n`);
+      process.stdout.write('    Invoke your functions and view logs in the dashboard\n');
+      process.stdout.write(`\n  Run ${chalk.bold('serverless deploy')} in the project directory\n`);
+      process.stdout.write(
+        "    Redeploy your service after you've updated your service code or configuration\n\n"
+      );


nit: Maintainance wise it might be nicer to construct it with one process.stdout.write but simply group messages in [].join("\n") (also we may resort to template string, still then indentation could get quirky)

Good call 👍

lib/cli/interactive-setup/deploy.js

medikoo · 2021-06-09T09:49:16Z

lib/plugins/deploy-interactive.js

@@ -0,0 +1,36 @@
+'use strict';


I'd keep this plugin in lib/cli/interactive-setup as it serves only interactive deploy case, and it's a temporary hack, that at some point will be removed

medikoo · 2021-06-09T09:56:21Z

lib/cli/interactive-setup/utils.js

+  isUserLoggedIn: () => {
+    if (process.env.SERVERLESS_ACCESS_KEY) {
+      return true;
+    }
+
+    if (configUtils.getLoggedInUser()) {
+      return true;
+    }
+
+    return false;
+  },
+  doesServiceInstanceHaveLinkedProvider: async ({ configuration, options }) => {
+    const region = resolveRegion({ configuration, options });
+    const stage = resolveStage({ configuration, options });
+    let result;
+    try {
+      result = await resolveProviderCredentials({ configuration, region, stage });
+    } catch (err) {
+      if (err.statusCode && err.statusCode >= 500) {
+        throw new ServerlessError(
+          'Dashboard service is currently unavailable, please try again later',
+          'DASHBOARD_UNAVAILABLE'
+        );
+      }
+      throw err;
+    }
+
+    return Boolean(result);
+  },


Those utils feel to me as dashboard-plugin utils, they're not specific to interactive CLI

Do you suggest to move them to dashboard-plugin then?

Yes, I think it's a better fit, and I can imagine they will then become useful in other logic points

As discussed previously, I've switched to use isAuthenticated from the plugin but kept the other util here as we would still have to develop a similar wrapper for that functionality on Framework side.

medikoo · 2021-06-09T09:57:07Z

package.json

@@ -61,6 +62,7 @@
    "node-fetch": "^2.6.1",
    "object-hash": "^2.2.0",
    "path2": "^0.1.0",
+    "process-utils": "^4.0.0",


Let's also remove it from devDependencies

medikoo

Looks great 👍

pgrzesik added enhancement cat/cli-onboarding Interactive CLI onboarding labels Jun 1, 2021

pgrzesik self-assigned this Jun 1, 2021

pgrzesik force-pushed the add-deploy-to-interactive branch from be72645 to dc3ba46 Compare June 1, 2021 09:44

pgrzesik marked this pull request as draft June 1, 2021 10:13

medikoo reviewed Jun 1, 2021

View reviewed changes

pgrzesik force-pushed the add-deploy-to-interactive branch from dc3ba46 to 22f40a4 Compare June 2, 2021 14:18

pgrzesik requested a review from medikoo June 2, 2021 14:25

pgrzesik force-pushed the add-deploy-to-interactive branch from 22f40a4 to 75c4458 Compare June 4, 2021 14:07

pgrzesik mentioned this pull request Jun 4, 2021

Add getDashboardInteractUrl util serverless/dashboard-plugin#593

Merged

pgrzesik changed the title ~~feat(CLI Onboarding): Add deploy step [WIP]~~ feat(CLI Onboarding): Add deploy step Jun 4, 2021

pgrzesik marked this pull request as ready for review June 4, 2021 14:25

medikoo reviewed Jun 5, 2021

View reviewed changes

pgrzesik marked this pull request as draft June 7, 2021 19:14

pgrzesik force-pushed the add-deploy-to-interactive branch from 75c4458 to 841817f Compare June 9, 2021 09:22

pgrzesik marked this pull request as ready for review June 9, 2021 09:31

pgrzesik changed the base branch from master to support-provider-setup-flow June 9, 2021 09:31

pgrzesik force-pushed the add-deploy-to-interactive branch from 841817f to 1fd61de Compare June 9, 2021 09:34

pgrzesik requested a review from medikoo June 9, 2021 09:38

medikoo reviewed Jun 9, 2021

View reviewed changes

pgrzesik force-pushed the add-deploy-to-interactive branch from 1fd61de to a9953e3 Compare June 11, 2021 07:45

pgrzesik force-pushed the support-provider-setup-flow branch from 8541b45 to 4c06d75 Compare June 28, 2021 09:25

Base automatically changed from support-provider-setup-flow to master June 28, 2021 09:54

pgrzesik force-pushed the add-deploy-to-interactive branch from a9953e3 to 4e9f59b Compare June 28, 2021 10:39

feat(CLI Onboarding): Add deploy step

2768e9f

pgrzesik requested a review from medikoo June 28, 2021 10:57

pgrzesik force-pushed the add-deploy-to-interactive branch from 4e9f59b to 2768e9f Compare June 28, 2021 10:59

medikoo approved these changes Jun 28, 2021

View reviewed changes

pgrzesik merged commit 28a06a0 into master Jun 28, 2021

pgrzesik deleted the add-deploy-to-interactive branch June 28, 2021 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(CLI Onboarding): Add deploy step #9536

feat(CLI Onboarding): Add deploy step #9536

pgrzesik commented Jun 1, 2021 •

edited

medikoo Jun 1, 2021

pgrzesik Jun 2, 2021

medikoo Jun 1, 2021

pgrzesik Jun 2, 2021

medikoo Jun 5, 2021

pgrzesik Jun 7, 2021

pgrzesik commented Jun 2, 2021

medikoo left a comment

medikoo Jun 5, 2021

codecov bot commented Jun 9, 2021 •

edited

pgrzesik commented Jun 9, 2021

medikoo left a comment

medikoo Jun 9, 2021

pgrzesik Jun 9, 2021

medikoo Jun 9, 2021

pgrzesik Jun 9, 2021

medikoo Jun 9, 2021

pgrzesik Jun 9, 2021

medikoo Jun 9, 2021

pgrzesik Jun 28, 2021

medikoo Jun 9, 2021

medikoo left a comment

feat(CLI Onboarding): Add deploy step #9536

feat(CLI Onboarding): Add deploy step #9536

Conversation

pgrzesik commented Jun 1, 2021 • edited

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 Jun 2, 2021

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 9, 2021 • edited

Codecov Report

pgrzesik commented Jun 9, 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

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

pgrzesik commented Jun 1, 2021 •

edited

codecov bot commented Jun 9, 2021 •

edited