Skip to content
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

Merged
merged 2 commits into from
Jun 8, 2021

Conversation

pgrzesik
Copy link
Contributor

@pgrzesik pgrzesik commented Jun 4, 2021

Adds getDashboardInteractUrl util + improves formatting of success message in dashboard-set-org step of Onboarding CLI

Addresses: serverless/serverless#9367

Related to: serverless/serverless#9536

@pgrzesik pgrzesik added the enhancement New feature or request label Jun 4, 2021
@pgrzesik pgrzesik self-assigned this Jun 4, 2021
@pgrzesik pgrzesik force-pushed the fix-formatting-for-dashboard-set-org-step branch 2 times, most recently from bac5620 to d1c3a38 Compare June 7, 2021 07:49
@codecov
Copy link

codecov bot commented Jun 7, 2021

Codecov Report

Merging #593 (23809b0) into master (cb7caa6) will increase coverage by 0.00%.
The diff coverage is 60.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #593   +/-   ##
=======================================
  Coverage   48.28%   48.28%           
=======================================
  Files          87       87           
  Lines        2802     2806    +4     
=======================================
+ Hits         1353     1355    +2     
- Misses       1449     1451    +2     
Impacted Files Coverage Δ
lib/cli/interactive-setup/dashboard-set-org.js 96.03% <ø> (ø)
lib/dashboard.js 77.77% <60.00%> (-22.23%) ⬇️

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 cb7caa6...23809b0. Read the comment docs.

@pgrzesik pgrzesik requested a review from medikoo June 7, 2021 11:23
Copy link
Contributor

@medikoo medikoo left a 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;
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion 👍

@pgrzesik pgrzesik force-pushed the fix-formatting-for-dashboard-set-org-step branch from d1c3a38 to e1fbe09 Compare June 7, 2021 14:52
@pgrzesik pgrzesik requested a review from medikoo June 7, 2021 16:02
Copy link
Contributor

@medikoo medikoo left a 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(
Copy link
Contributor

@medikoo medikoo Jun 7, 2021

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.

Copy link
Contributor Author

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 👍

@pgrzesik pgrzesik force-pushed the fix-formatting-for-dashboard-set-org-step branch from e1fbe09 to 23809b0 Compare June 8, 2021 08:46
@pgrzesik pgrzesik requested a review from medikoo June 8, 2021 09:08
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 👍

@pgrzesik pgrzesik merged commit 0182e87 into master Jun 8, 2021
@pgrzesik pgrzesik deleted the fix-formatting-for-dashboard-set-org-step branch June 8, 2021 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants