Built in integration of serverless enterprise #6074

dschep · 2019-05-01T20:23:28Z

What did you implement:

PLAT-916

How did you implement it:

Added SFE plugin as a dependency and automatically load it, explicitly from sls's node_modules to avoid issues with global installation.

How can we verify it:

npm i -g serverless/serverless#plat-916

if you have logged into SFE, first remove those details from or remove the ~/.serverlessrc file entirely. Then use sls to do some deploys, you should get the ad about SFE on deploys.

then run sls login and deploy again, you should get warnings about configuring app&tenant.

configure app&tenant and deploy again. should now be using SFE!

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

Looks good so far @dschep 👍

Just added one comment regarding path joining...

lib/classes/PluginManager.js

package.json

medikoo · 2019-05-15T11:54:15Z

.travis.yml

-  - node_js: '4.4'
-    env: SLS_IGNORE_WARNING=*
-  - node_js: '5.11'
-    env: SLS_IGNORE_WARNING=*


I would leave it to #6077 (still I guess it was probably done to have CI pass)

Yes, either that has to be merged first or I have to do it here too.

medikoo · 2019-05-15T11:54:58Z

.travis.yml

@@ -42,6 +38,7 @@ matrix:
    - LINTING=true
 sudo: false
 install:
+- travis_retry npm install -g npm


Why? (line below it's followed by npm install)

npm install doesn't upgrade npm and some dep of enterprise plugin requires a new version of npm to install correctly.

Isn't that related purely to npm version that comes with Node.js v4? (of which testing we drop)

i'll try removing it but i think other versions needed update dnpm too

I think once testing of v4 and v5 is dropped, it should not be an issue (and better to drop it, as it adds time to CI build, and installs npm version not yet "approved" by Node.js)

this already drops node 4&5, it's still needed on node 6

medikoo · 2019-05-15T12:03:10Z

lib/classes/PluginManager.js

@@ -148,7 +152,27 @@ class PluginManager {
    if (pluginsObject.localPath) {
      module.paths.unshift(pluginsObject.localPath);
    }
-    this.loadPlugins(pluginsObject.modules);
+    this.loadPlugins(pluginsObject.modules
+      .filter(name => name !== '@serverless/enterprise-plugin'));


Is it just to prevent accidental load of enterprise plugin if user lists @serverless/enterprise-plugin in serverless.yml plugins section?

Is it because we don't want to load plugins twice? In that case we already have (or had) that logic in the plugin manager.

Maybe the logic related to the @serverless/enterprise-plugin is an edge case though...

It's probably good to treat it separately here, but I guess it'll be good to show some warning to the user, that @serverless/enterprise-plugin should not be listed as plugin (and carry on as normal).

That way we don't leave an impression that such configuration of a plugin is expected.

lib/classes/PluginManager.js

package.json

dschep · 2019-05-22T12:17:48Z

Is it because we don't want to load plugins twice? In that case we already have (or had) that logic in the plugin manager.

Maybe the logic related to the @serverless/enterprise-plugin is an edge case though...

@pmuens i can't seem to reply directly. but yes, we don't want to throw a warning about the duplicate either.

dschep · 2019-05-22T12:19:08Z

@medikoo it is intentional to load after other plugins. This is because it has to be after plugins like webpack or other transpilation tools.

medikoo

It needs conflicts to be resolved

medikoo · 2019-05-23T19:28:21Z

.travis.yml

@@ -36,6 +36,7 @@ matrix:
      env: SLS_IGNORE_WARNING=*
 sudo: false
 install:
+  - "[ $TRAVIS_OS_NAME = windows ] || travis_retry npm install -g npm"


Why is it needed just in Windows case?

npm i -g npm doesn't work on windows travis builders bc apparently the user doesn't have enough permission, and it's not necessary bc the windows build has a new enough node/npm that it doesn't need to be upgraded.

As we clarified on Slack, it's about invalid listing of zlib in package.json. it's old module that doesn't compile on new node.js versions (recent ones totally ignore it), and crashes v6.

zlib is a Node.js native module since v0.12.

Removing it from package.json will fix it

medikoo · 2019-05-24T06:46:00Z

lib/classes/PluginManager.js

@@ -148,7 +152,27 @@ class PluginManager {
    if (pluginsObject.localPath) {
      module.paths.unshift(pluginsObject.localPath);
    }
-    this.loadPlugins(pluginsObject.modules);
+    this.loadPlugins(pluginsObject.modules
+      .filter(name => name !== '@serverless/enterprise-plugin'));


It's probably good to treat it separately here, but I guess it'll be good to show some warning to the user, that @serverless/enterprise-plugin should not be listed as plugin (and carry on as normal).

That way we don't leave an impression that such configuration of a plugin is expected.

medikoo · 2019-05-24T06:48:28Z

lib/plugins/create/templates/aws-alexa-typescript/serverless.yml

@@ -1,5 +1,7 @@
 service:
  name: aws-alexa-typescript
+#app: your-app-name


I guess it's about credentials to Enterprise account. Wouldn't it be better to accompany it with comment as e.g.:

# A credentials to eventual Serverless Enterprise account # see https://serverless.com/acceleration/ for more details

Otherwise it gives no hint what those settings are about

it's not a credential, but yeah, i'll check with product what copy they'd like

Pretty sure it is, but is this only required for our AWS templates right now?

Correct. SFE only supports AWS.

lib/utils/config/index.js

package.json

lib/classes/PluginManager.js

…alls with old versions of npm

pmuens

LGTM from a code perspective

Just added 2 minor questions.

pmuens · 2019-05-28T11:57:44Z

lib/classes/PluginManager.js

+    if (config.getGlobalConfig().enterpriseDisabled) {
+      return;
+    }
+    module.paths.unshift(path.join(__dirname, '../../node_modules'));


I might've asked this before, but this will work on Windows, since path.join is able to turn / into \, correct?

oh right.. i tested this when i did it the other way (hacking it into a relative import). I think this'll work, but I'll boot up my window sbox to double check

pmuens · 2019-05-28T11:58:56Z

lib/plugins/create/templates/aws-alexa-typescript/serverless.yml

@@ -1,5 +1,7 @@
 service:
  name: aws-alexa-typescript
+#app: your-app-name


Pretty sure it is, but is this only required for our AWS templates right now?

dschep added 2 commits May 1, 2019 15:48

built in integration of serverless enterprise

c06a678

reference sfe plugin from npm, not local tarball

dccec55

pmuens reviewed May 2, 2019

View reviewed changes

lib/classes/PluginManager.js Outdated Show resolved Hide resolved

pmuens changed the title ~~built in integration of serverless enterprise~~ Built in integration of serverless enterprise May 2, 2019

pmuens added cat/dashboard feature labels May 2, 2019

dschep mentioned this pull request May 3, 2019

Update required Node.js version / Add version check #6077

Merged

5 tasks

dschep added 7 commits May 3, 2019 09:16

avoid duplicate plugin issue w/SFE

6914063

add option to disable SFE

2a3d039

check that node version is compatible with SFE plugin

b7d0c53

check for version of node that the SFEplugin requires too

ab88c1a

serverlessrc based config instead of env var

da1587b

TEMPORARY - switching SFE plugin dep to next tag to be able to test

6e09241

upgrade npm

aaf0dc6

pmuens reviewed May 6, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

remove obsolete node versions from travis

4634632

dschep marked this pull request as ready for review May 6, 2019 13:07

pmuens added pr/in-review and removed pr/in-progress labels May 7, 2019

dschep added 3 commits May 8, 2019 09:21

update check

049d147

Merge remote-tracking branch 'origin/master' into plat-916

68b9c39

switch to latest

856e0ec

dschep requested review from pmuens, medikoo and eahefnawy May 14, 2019 18:04

medikoo reviewed May 15, 2019

View reviewed changes

dschep added 3 commits May 20, 2019 09:13

try not upgrading npm

581d647

update AWS templates with sfe app/tenant placeholders

ef728aa

remove sfe node version check

c17196a

dschep added 3 commits May 23, 2019 08:39

sfe bump

9fc9d20

Merge branch 'master' into plat-916

df5cfd4

lint

8206512

dschep requested a review from medikoo May 23, 2019 13:16

medikoo reviewed May 23, 2019

View reviewed changes

dschep added 5 commits May 23, 2019 09:41

Merge remote-tracking branch 'origin/master' into plat-916

8abd152

don't upgrade npm on windows

6ab06eb

wtf travis

fc5bb75

oh. stupid yaml broke travis parsing of config

9df621e

upgrade npm before using it

43eb300

dschep requested review from medikoo and pmuens and removed request for medikoo May 23, 2019 16:41

version bumps

c675900

medikoo reviewed May 24, 2019

View reviewed changes

pmuens added this to the 1.44.0 milestone May 24, 2019

pmuens reviewed May 24, 2019

View reviewed changes

lib/utils/config/index.js Show resolved Hide resolved

package.json Show resolved Hide resolved

medikoo reviewed May 24, 2019

View reviewed changes

lib/classes/PluginManager.js Outdated Show resolved Hide resolved

dschep added 6 commits May 24, 2019 12:47

Merge remote-tracking branch 'origin/master' into plat-916

c636c8d

remove unneeded eslint directive

18befe0

re add enterprise disable d sflag support

9e4621c

SFE plugin 1.0.3 removed unneeded zlib dep that was breaking npm inst…

e335bdb

…alls with old versions of npm

Merge remote-tracking branch 'origin/master' into plat-916

197ca06

Update changelog

5f6cd03

pmuens approved these changes May 28, 2019

View reviewed changes

dschep merged commit 224669d into master May 28, 2019

medikoo deleted the plat-916 branch December 10, 2019 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Built in integration of serverless enterprise #6074

Built in integration of serverless enterprise #6074

dschep commented May 1, 2019 •

edited

Loading

pmuens left a comment

medikoo May 15, 2019

dschep May 20, 2019

medikoo May 15, 2019

dschep May 20, 2019

medikoo May 20, 2019

dschep May 20, 2019

medikoo May 20, 2019

dschep May 22, 2019

medikoo May 15, 2019

dschep May 20, 2019

pmuens May 22, 2019

medikoo May 24, 2019

dschep commented May 22, 2019

dschep commented May 22, 2019

medikoo left a comment

medikoo May 23, 2019

dschep May 24, 2019

medikoo May 24, 2019

medikoo May 24, 2019

medikoo May 24, 2019

dschep May 24, 2019

pmuens May 28, 2019

dschep May 28, 2019

pmuens left a comment

pmuens May 28, 2019

dschep May 28, 2019

dschep May 28, 2019

pmuens May 28, 2019

Built in integration of serverless enterprise #6074

Built in integration of serverless enterprise #6074

Conversation

dschep commented May 1, 2019 • edited Loading

What did you implement:

How did you implement it:

How can we verify it:

Todos:

pmuens 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschep commented May 22, 2019

dschep commented May 22, 2019

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

pmuens 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

dschep commented May 1, 2019 •

edited

Loading