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.

Sign up for GitHub

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

Jump to bottom

feat(CLI Onboarding): Auto login if --org flag provided #10510

Merged
merged 1 commit into from Jan 14, 2022
Merged

feat(CLI Onboarding): Auto login if --org flag provided #10510

merged 1 commit into from Jan 14, 2022

Conversation

pgrzesik
Copy link
Contributor

Reported internally, related to changes in onboarding flow.

Question: Should we also have similar behavior if it's existing service with org configured already in service config? @medikoo @mnapoli

@pgrzesik pgrzesik added enhancement cat/cli-onboarding Interactive CLI onboarding labels Jan 14, 2022
@pgrzesik pgrzesik self-assigned this Jan 14, 2022
@codecov
Copy link

codecov bot commented Jan 14, 2022 •
edited

Codecov Report

Merging #10510 (d7f5466) into master (427920e) will decrease coverage by 0.00%.
The diff coverage is 93.33%.

❗ Current head d7f5466 differs from pull request most recent head 63d46f2. Consider uploading reports for the commit 63d46f2 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10510      +/-   ##
==========================================
- Coverage   85.83%   85.83%   -0.01%     
==========================================
  Files         335      335              
  Lines       13930    13937       +7     
==========================================
+ Hits        11957    11963       +6     
- Misses       1973     1974       +1
Impacted Files Coverage Δ
lib/plugins/aws/info/display.js 75.00% <50.00%> (-0.68%) ⬇️
lib/cli/interactive-setup/dashboard-login.js 88.09% <100.00%> (+0.59%) ⬆️
lib/cli/interactive-setup/dashboard-set-org.js 95.03% <100.00%> (ø)
lib/utils/logDeprecation.js 100.00% <100.00%> (ø)

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 427920e...63d46f2. Read the comment docs.

@pgrzesik pgrzesik requested a review from medikoo January 14, 2022 10:56
@medikoo
Copy link
Contributor

medikoo commented Jan 14, 2022

Question: Should we also have similar behavior if it's existing service with org configured already in service config? @medikoo @mnapoli

I think yes, and if I see correctly, this change is proposed in that way (?)

medikoo
medikoo previously approved these changes Jan 14, 2022
View reviewed changes
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
Copy link
Contributor Author

Question: Should we also have similar behavior if it's existing service with org configured already in service config? @medikoo @mnapoli

I think yes, and if I see correctly, this change is proposed in that way (?)

Nope, the proposed version only checks org options - what do you think @mnapoli? I agree that we should also have that behavior for org in service configuration

@mnapoli
Copy link
Contributor

mnapoli commented Jan 14, 2022

👍 agreed, the same behavior should apply in that case.

@pgrzesik pgrzesik force-pushed the automatically-login-if-org-provided branch from 0bc890e to a1679e0 Compare January 14, 2022 12:49
@pgrzesik pgrzesik requested a review from medikoo January 14, 2022 16:16
medikoo
medikoo reviewed Jan 14, 2022
View reviewed changes
lib/cli/interactive-setup/dashboard-login.js Outdated
Comment on lines 22 to 24
Boolean(context.options.org) ||
Boolean(context.configuration.org) ||
(await loginOrRegisterQuestion(context.stepHistory));
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need for Boolean wrappers.

Sometimes it's good to ensure that given property, or function result, consistently represents boolean (it's also about not holding onto some objects which otherwise could be cleared from memory), but here it's just part of internal conditional.

lib/cli/interactive-setup/dashboard-login.js Outdated
'Enable Serverless Dashboard to get enhanced monitoring, logs and secrets management: https://serverless.com/monitoring\n\n'
);
if (context.initial.isInServiceContext && !context.initial.isDashboardEnabled) {
const isOrgProvided = Boolean(context.options.org) || Boolean(context.configuration.org);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@pgrzesik pgrzesik force-pushed the automatically-login-if-org-provided branch from a1679e0 to 63d46f2 Compare January 14, 2022 16:32
@pgrzesik pgrzesik requested a review from medikoo January 14, 2022 16:49
medikoo
medikoo approved these changes Jan 14, 2022
View reviewed changes
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 pgrzesik merged commit dcf5273 into master Jan 14, 2022
@pgrzesik pgrzesik deleted the automatically-login-if-org-provided branch January 14, 2022 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@medikoo medikoo medikoo approved these changes

Assignees

@pgrzesik pgrzesik

Labels
cat/cli-onboarding Interactive CLI onboarding enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pgrzesik @medikoo @mnapoli