Add login command #3558

DavidWells · 2017-05-04T23:28:20Z

What did you implement:

Adds serverless login to the CLI.

Running this command optional & is prep work for the upcoming release of the serverless platform.

Additionally, this is part of an ongoing effort to improve the developer experience of the CLI. The login command will help us help new developers adopt & use the framework.

How did you implement it:

Added login command

How can we verify it:

Run sls login, login, grab your token, & insert back into the CLI.

Todos:

Is this ready for review?: Yes
Is it a breaking change?: No

pmuens

Nice @DavidWells 👍 Really looking forward to merge this PR!

I added some comments. It's mostly that some of the functionality could be moved and some utils the Serverless core provides can be used. This way we can remove some utils code in here.

Let me know if you need any help here! 💯

pmuens · 2017-05-05T11:25:11Z

lib/plugins/login/login.js

+const getFrameworkId = require('../../utils/getFrameworkId');
+const updateConfig = require('../../utils/config/update');
+
+const config = {


Will those values stay hard coded once merged?

Would be nice if they could be set via env variables.

Here is okay for now. We can move to centralized spot once more default config stuff arises

hmmm I'm super curious about that. Would anyone be able to just copy that into their code and implement their own login? Do we want people logged in outside of the context of the framework?

pmuens · 2017-05-05T11:27:04Z

lib/plugins/login/login.js

+  }
+  openBrowser(url) {
+    let browser = process.env.BROWSER;
+    if (browser === 'none') {


Will this really return none if not set or something like null / undefined. In my Node REPL it returns undefined.

pmuens · 2017-05-05T11:29:55Z

lib/plugins/login/login.js

+      opn(url, options).catch(() => {});
+      return true;
+    } catch (err) {
+      console.log('---------------------------'); // eslint-disable-line


you could use this.serverless.cli.log or this.serverless.cli.consoleLog to get rid of the eslint disables.

waiting on #3558 to merge implement

DavidWells · 2017-05-05T22:25:18Z

@brianneisler I don't see the signatures changing much. That would also break all the tests.

IMO, We can refactor to mixin approach later if we need it.

updated the utils class to use util functions

brianneisler · 2017-05-05T22:39:05Z

@DavidWells how does that break the tests?

DavidWells · 2017-05-05T23:05:37Z

Changing Params will break tests

…

On May 5, 2017, at 3:39 PM, Brian Neisler ***@***.***> wrote: @DavidWells how does that break the tests? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

brianneisler · 2017-05-10T00:28:48Z

@DavidWells could we update this to use the rc module. This will give us #3573 out of the box and a host of other goodies.

DavidWells · 2017-05-10T00:52:39Z

rc doesnt support our proposed path of ./serverless/.serverlessrc

Giving ultimate flexibility with where config can live might hurt us later (IE rc automagically parses args and people could have conflicting arg keys as one example)

I'd propose a simpler solution for right now. ./serverless/.serverlessrc and local .serverlessrc only. We could expand to other options if people really need other options

pmuens · 2017-05-11T13:30:47Z

Just a quick reminder that now that we should add docs for the login command to the Google docs now that they were merged (see todo-list in: #3365 (comment)).

eahefnawy

Looking good. Just couple of comments 😊

eahefnawy · 2017-05-12T12:33:38Z

lib/classes/Utils.js

-      return false;
-    }
+    // console.log('this.serverless.utils.fileExistsSync being depricated in a future release')
+    // console.log('Please explicitly declare your plugin dependancies')


hmmm why are we deprecating this? and why is it commented out?

This will be deprecated since we're moving towards a more functional codebase in the future where we can easily test all the different pieces independently w/o having to pass around this huge serverless object.

I believe those are preparations so that this can be commented out once v2 will be around the corner.

eahefnawy · 2017-05-12T12:36:15Z

lib/plugins/login/login.js

+const getFrameworkId = require('../../utils/getFrameworkId');
+const updateConfig = require('../../utils/config/update');
+
+const config = {


hmmm I'm super curious about that. Would anyone be able to just copy that into their code and implement their own login? Do we want people logged in outside of the context of the framework?

eahefnawy · 2017-05-12T12:37:27Z

lib/plugins/login/login.js

+      return true;
+    } catch (err) {
+      console.log('---------------------------'); // eslint-disable-line
+      console.log(`🙈  ${chalk.red("Unable to open browser automatically")}`); // eslint-disable-line


love that crazy monkey! ☺️

Side Note: I don't have much experience with emojis on the CLI, but if we can really do that without side effects, we should probably use ⚡️ somewhere in there, maybe replace that ugly Serverless: prompt on every line.

We'll change the logo soon though...

I can remember that @nikgraf changed the ... to the unicode representation which "broke" the CLI displaying for some people. So I'm not sure if this is a good idea --> See: #2714

It just rendered poorly for people without unicode support (which I'd guesstimate is low)

Serverless: Packaging serviceΓÇª

yarn uses emojis, I think we can use them too. Looks like they use a package to do so reliably https://github.com/yarnpkg/yarn/blob/cb453f56353016d5c98e4d6a0f4069896b1eef43/src/cli/commands/why.js#L17 . I will look into this

eahefnawy · 2017-05-12T12:39:49Z

lib/plugins/login/login.js

+        })
+          .then((response) => response.json())
+          .then((platformResponse) => {
+            // console.log('data', platformResponse);


I'm guessing this is left out from debugging?

yea will be removed

pmuens

Looking good so far. 👍

Just added some comments while looking through the code.

pmuens · 2017-05-24T06:20:21Z

docs/providers/azure/cli-reference/login.md

@@ -0,0 +1,24 @@
+<!--
+title: Serverless Framework Commands - Login
+menuText: Login


should we lowercase this so that it follows the new naming schema?

pmuens · 2017-05-24T06:20:44Z

docs/providers/aws/cli-reference/login.md

@@ -0,0 +1,24 @@
+<!--
+title: Serverless Framework Commands - Login
+menuText: Login


should we lowercase this so that it follows the new naming schema?

pmuens · 2017-05-24T06:20:55Z

docs/providers/openwhisk/cli-reference/login.md

@@ -0,0 +1,24 @@
+<!--
+title: Serverless Framework Commands - Login
+menuText: Login


should we lowercase this so that it follows the new naming schema?

yes =). Will update

pmuens · 2017-05-24T06:24:57Z

lib/plugins/login/login.js

+                id,
+                email: decoded.email,
+              }).then(() => {
+                // console.log('login complete');


This could be removed.

pmuens · 2017-05-24T06:25:33Z

lib/plugins/login/login.js

+            });
+          })
+          .catch(() => {
+            console.log(chalk.red('Incorrect token value supplied. Please run `serverless login` again')); // eslint-disable-line


Maybe we can wrap serverless login into "" since the console output won't render markdown (the tildes).

Updated it. Is that a windows thing?

pmuens · 2017-05-24T06:26:48Z

lib/plugins/logout/logout.js

+          configUtils.set(`users.${currentId}.auth`, null);
+          // log stat
+          userStats.track('user_loggedOut').then(() => {
+            console.log('Successfully logged out.'); // eslint-disable-line


Best would be if this.serverless.cli.consoleLog is used.

pmuens · 2017-05-24T06:29:56Z

lib/utils/userStats.js

+  if (DEBUG) {
+    console.log('.track call', eventData); // eslint-disable-line
+  }
+  // return BbPromise.resolve();


This can be removed.

pmuens · 2017-05-24T06:29:58Z

lib/utils/userStats.js

+
+  const eventData = _.merge(defaultData, data);
+  if (DEBUG) {
+    console.log('.track call', eventData); // eslint-disable-line


Best would be if this.serverless.cli.consoleLog is used.

pmuens · 2017-05-24T06:33:58Z

lib/utils/config/index.js

+const path = require('path');
+const os = require('os');
+const _ = require('lodash');
+const writeFileAtomic = require('write-file-atomic');


Is there a way to e.g. use some stuff from fs-extra or any other method already available so that we don't introduce yet another dependency?

pmuens · 2017-05-24T11:37:47Z

Ok. Travis is a liar. The tests are failing. Working on an update...

pmuens

Just pushed a couple of changes and tested the login / logout functionality.

Works like a charm. GTM from my side!

@eahefnawy could you take a second look and merge if everything is fine? @DavidWells changed some stuff in the meantime. Thanks!

eahefnawy

Just gave one final review. Looking great! Hopefully we slowly move all our utils to follow this pattern.

Thanks @DavidWells 😊

waiting on #3558 to merge implement

pmuens requested review from pmuens and eahefnawy May 5, 2017 05:31

pmuens added the stage/in-progress label May 5, 2017

pmuens previously requested changes May 5, 2017

View reviewed changes

DavidWells added a commit that referenced this pull request May 5, 2017

add conditional check

8393ea4

waiting on #3558 to merge implement

DavidWells mentioned this pull request May 5, 2017

DX: Add error tracking #3564

Merged

pmuens added this to the 1.14 milestone May 8, 2017

pmuens mentioned this pull request May 8, 2017

Re-Add Google Cloud Functions documentation #3365

Merged

5 tasks

This was referenced May 8, 2017

Add project level support for .serverlessrc #3573

Closed

Add sls config commands #3574

Closed

eahefnawy approved these changes May 12, 2017

View reviewed changes

pmuens reviewed May 24, 2017

View reviewed changes

DavidWells added 11 commits May 24, 2017 00:13

add login command

754bfe3

add functional Util functions ported from Utils class

7016d41

add login deps

51da948

use strict

dc4f389

add docs

24606e1

update Utils class with util functions

e93e976

add logout command

40dee9b

update config util

82fd943

update login cmd

c7db666

update linting

4bef14d

add tests

b5c0a33

DavidWells added 7 commits May 24, 2017 00:21

update util

33dca7a

update login

4c75ce8

update for serverlessrc

0703677

update template creation

93ed02b

update deploy

095bf4b

sync deps

263ee54

remove old dep

d310572

DavidWells force-pushed the addLogin branch from 99a17e2 to d310572 Compare May 24, 2017 07:34

DavidWells and others added 10 commits May 24, 2017 00:36

shrink wrap

82b4480

update docs

78bf0a2

add google doc

3072390

change tildes

9e7a1e9

update test

710ff6f

update test

1b8b8ae

add createdTimestamp

e97285a

add file meta

b4aced3

add empty obj

ac7515f

Add login CLI command to TOCs

3f1c8b4

pmuens added stage/in-review and removed stage/in-progress labels May 24, 2017

pmuens added 2 commits May 24, 2017 13:01

Remove commented out code

a5d81e0

Update console.log statements and add file based eslint ignore comments

9dd5dc3

pmuens added 2 commits May 24, 2017 14:01

Fix broken Utils tests

a663974

Shrinkwrapping

f9e3a99

pmuens approved these changes May 24, 2017

View reviewed changes

eahefnawy approved these changes May 24, 2017

View reviewed changes

eahefnawy merged commit bb7700d into master May 24, 2017

eahefnawy deleted the addLogin branch May 24, 2017 13:02

DavidWells added a commit that referenced this pull request May 26, 2017

add conditional check

6efa28e

waiting on #3558 to merge implement

Add login command #3558

Add login command #3558

Conversation

DavidWells commented May 4, 2017 • edited by pmuens

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

DavidWells commented May 5, 2017

brianneisler commented May 5, 2017

DavidWells commented May 5, 2017 via email

brianneisler commented May 10, 2017

DavidWells commented May 10, 2017

pmuens commented May 11, 2017 • edited

eahefnawy 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

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

pmuens May 24, 2017 • edited

Choose a reason for hiding this comment

pmuens commented May 24, 2017

pmuens left a comment

Choose a reason for hiding this comment

eahefnawy left a comment

Choose a reason for hiding this comment

DavidWells commented May 4, 2017 •

edited by pmuens

pmuens commented May 11, 2017 •

edited

pmuens May 24, 2017 •

edited