Improve interactive AWS creds flow #6449

dschep · 2019-07-25T17:54:25Z

What did you implement:

closes PLAT-1341

How did you implement it:

implemented by asking if the user has an account, if not, open browser to sign up
and then open browser (in new or existing acct case) to create an admin user for
user with serverless.

added open as a dependency to open the browser

How can we verify it:

npm i -g serverless/serverless#nicer-aws-creds-flow
mv ~/.aws ~/.awsbak
sls

Todos:

Note: Run npm run test-ci to run all validation checks on proposed changes

Write tests and confirm existing functionality is not broken.
Validate via npm test
Write documentation
Ensure there are no lint errors.
Validate via npm run lint-updated
Note: Some reported issues can be automatically fixed by running npm run lint:fix
Ensure introduced changes match Prettier formatting.
Validate via npm run prettier-check-updated
Note: All reported issues can be automatically fixed by running npm run prettify-updated
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?: NO
Is it a breaking change?: NO

closes PLAT-1341 implemented by asking if the user has an account, if not, open browser to sign up and then open browser (in new or existing acct case) to create an admin user for user with serverless. added open as a dependency to open the browser

dschep · 2019-07-25T19:03:30Z

Uggh... open>=6 depends on node >= 8 but open<6 fails npm audit. Any ideas?

pmuens

Uggh... open>=6 depends on node >= 8 but open<6 fails npm audit. Any ideas?

I looked for alternatives, but apparently there are no up-to-date packages out there.

🤔 Maybe we can to fork open it and fix the dependency issue for version 6 on our own?

pmuens · 2019-07-26T11:47:54Z

lib/utils/openBrowser.js

+  console.log('---------------------------');
+  const errMsg = err ? `\nError: ${err.message}` : '';
+  const msg = `Unable to open browser automatically${errMsg}`;
+  console.log(`🙈  ${chalk.red(msg)}`);


While cute I'm unsure if we want to go the "emotification of all the CLIs" route in an enterprise-facing project.

I copy and pasted this out of platform-sdk 😬

(as to why i copied, it.. didn't want to add sdk as a direct dep, which is the only way to guarantee it's availability with things like yarn, and now also the fact it requires newer versions of node)

now also the fact it requires newer versions of node

But SDK Is dependency of a plugin.. so if SDK was updated to not work in v6, then through Plugin it'll also break SLS (?)

dschep · 2019-07-26T12:17:31Z

🤔 Maybe we can to fork open it and fix the dependency issue for version 6 on our own?

Yeah.. It's a single file AFAICT https://github.com/sindresorhus/open/blob/master/index.js So i'll do that.

pmuens

This looks really good.

The only issue I ran into was that the focus was switched to my browser when I answered the question "Serverless: Do you want to set them up now? Yes". The problem is that there was no website opened. Only the browser was focused. The steps after that opened the browser URLs correctly.

Here are the logs:

[~/Desktop/code.nosync/serverless/tmp]$ ../bin/serverless

Serverless: No project detected. Do you want to create a new one? Yes
Serverless: What do you want to make? AWS Node.js
Serverless: What do you want to call this project? foo

Project successfully created in 'foo' folder.

No AWS credentials were found on your computer, you need these to host your application.

Serverless: Do you want to set them up now? Yes
Serverless: Do you have an AWS account? No
Serverless: Press Enter to continue after creating an AWS account 
Serverless: Press Enter to continue after creating an AWS user with access keys 
Serverless: AWS Access Key Id:

Safari is my primary browser.

skierkowski

This is such a slick experience.

pmuens

Just tested this again to see if it's a glitch but I can consistently reproduce the problem that the default Broswer (Safari in my case) is opened without opening a new tab / window (see comment above).

@medikoo can you take this for a spin and see if you're also facing the same problems? I'm wondering if it's just Safari which has problems here.

medikoo

Looks like really good improvement. Just minor remarks.

medikoo · 2019-08-08T13:18:12Z

lib/utils/openBrowser.js

+
+function displayManualOpenMessage(url, err) {
+  // https://github.com/sindresorhus/log-symbols
+  console.log('---------------------------');


Why we do not rely here on serverless logger? Even if we don't like to use cli.log we may rel on cli.consoleLog.

It'll clearly indicate the intention of generating user facing CLI output.

Direct use of console.log is more for temporary debug output (which we prevent from committing in via lint rules)

medikoo · 2019-08-08T13:25:22Z

lib/utils/openBrowser.js

+  console.log('---------------------------');
+  const errMsg = err ? `\nError: ${err.message}` : '';
+  const msg = `Unable to open browser automatically${errMsg}`;
+  console.log(`🙈  ${chalk.red(msg)}`);


now also the fact it requires newer versions of node

But SDK Is dependency of a plugin.. so if SDK was updated to not work in v6, then through Plugin it'll also break SLS (?)

Imporove interactive AWS creds flow

e6367e0

closes PLAT-1341 implemented by asking if the user has an account, if not, open browser to sign up and then open browser (in new or existing acct case) to create an admin user for user with serverless. added open as a dependency to open the browser

dschep requested review from skierkowski and pmuens July 25, 2019 17:54

pmuens added cat/providers enhancement labels Jul 26, 2019

pmuens changed the title ~~Imporove interactive AWS creds flow~~ Imporve interactive AWS creds flow Jul 26, 2019

pmuens reviewed Jul 26, 2019

View reviewed changes

pmuens changed the title ~~Imporve interactive AWS creds flow~~ Improve interactive AWS creds flow Jul 26, 2019

dschep added 2 commits July 26, 2019 09:31

lint

c44f9fa

go with a vendorized version of open adapted for node 6

9c816f2

dschep force-pushed the nicer-aws-creds-flow branch from 7c9ae42 to 9c816f2 Compare July 26, 2019 13:32

dschep added 2 commits July 26, 2019 09:36

open depends on is-wsl

70558a8

backport to promises. gross.

c6da38b

dschep marked this pull request as ready for review July 30, 2019 12:39

dschep requested a review from pmuens July 30, 2019 12:39

pmuens added pr/in-review and removed pr/in-progress labels Jul 30, 2019

pmuens reviewed Jul 30, 2019

View reviewed changes

skierkowski approved these changes Jul 31, 2019

View reviewed changes

pmuens reviewed Aug 8, 2019

View reviewed changes

pmuens requested a review from medikoo August 8, 2019 10:34

medikoo reviewed Aug 8, 2019

View reviewed changes

dschep merged commit f9c25db into master Aug 8, 2019

dschep deleted the nicer-aws-creds-flow branch August 8, 2019 14:05

medikoo added this to the 1.50.0 milestone Aug 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve interactive AWS creds flow #6449

Improve interactive AWS creds flow #6449

dschep commented Jul 25, 2019

dschep commented Jul 25, 2019

pmuens left a comment •

edited

pmuens Jul 26, 2019

dschep Jul 26, 2019

dschep Jul 26, 2019

medikoo Aug 8, 2019

dschep commented Jul 26, 2019

pmuens left a comment

skierkowski left a comment

pmuens left a comment

medikoo left a comment

medikoo Aug 8, 2019

medikoo Aug 8, 2019

Improve interactive AWS creds flow #6449

Improve interactive AWS creds flow #6449

Conversation

dschep commented Jul 25, 2019

What did you implement:

How did you implement it:

How can we verify it:

Todos:

dschep commented Jul 25, 2019

pmuens left a comment • edited

Choose a reason for hiding this comment

pmuens Jul 26, 2019

Choose a reason for hiding this comment

dschep Jul 26, 2019

Choose a reason for hiding this comment

dschep Jul 26, 2019

Choose a reason for hiding this comment

medikoo Aug 8, 2019

Choose a reason for hiding this comment

dschep commented Jul 26, 2019

pmuens left a comment

Choose a reason for hiding this comment

skierkowski left a comment

Choose a reason for hiding this comment

pmuens left a comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

medikoo Aug 8, 2019

Choose a reason for hiding this comment

medikoo Aug 8, 2019

Choose a reason for hiding this comment

pmuens left a comment •

edited