-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add getDashboardInteractUrl
util
#593
Conversation
bac5620
to
d1c3a38
Compare
Codecov Report
@@ Coverage Diff @@
## master #593 +/- ##
=======================================
Coverage 48.28% 48.28%
=======================================
Files 87 87
Lines 2802 2806 +4
=======================================
+ Hits 1353 1355 +2
- Misses 1449 1451 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pgrzesik looks good! I have just one logical concern
lib/dashboard.js
Outdated
module.exports.getDashboardUrl = getDashboardUrl; | ||
|
||
module.exports.getDashboardInteractUrl = (ctx) => { | ||
if (!ctx.sls.enterpriseEnabled) return dashboardUrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be more logical to return null
in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion 👍
d1c3a38
to
e1fbe09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @pgrzesik, I've noticed one more potential issue with tests (one I've stumbled on occasionally). Please see my comment
@@ -342,7 +343,9 @@ describe('lib/cli/interactive-setup/dashboard-set-org.test.js', function () { | |||
expect(context.configuration.org).to.equal('testinteractivecli'); | |||
expect(context.configuration.app).to.equal('other-app'); | |||
expect(stdoutData).to.include( | |||
'Your project has been setup with org: "testinteractivecli" and app: "other-app"' | |||
`${chalk.green('Your project has been setup with org: ')}${chalk.white.bold( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be safer to use strip-ansi
in such case (we already do it in some places), as otherwise if we run tests in environement which does not support colors those tests will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great suggestion 👍
e1fbe09
to
23809b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 👍
Adds
getDashboardInteractUrl
util + improves formatting of success message indashboard-set-org
step of Onboarding CLIAddresses: serverless/serverless#9367
Related to: serverless/serverless#9536