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

Record preliminary context for Interactive Setup #9736

Merged
merged 2 commits into from Jul 14, 2021
Merged

Record preliminary context for Interactive Setup #9736

merged 2 commits into from Jul 14, 2021

Conversation

pgrzesik
Copy link
Contributor

Addresses: #9367

@pgrzesik pgrzesik self-assigned this Jul 13, 2021
@pgrzesik pgrzesik force-pushed the resolve-preliminary-context-at-the-beginning-of-interactive branch from 290324c to 7b20014 Compare July 13, 2021 09:56
@codecov
Copy link

codecov bot commented Jul 13, 2021 •
edited

Codecov Report

Merging #9736 (181066f) into master (4eb366b) will increase coverage by 0.00%.
The diff coverage is 95.00%.

❗ Current head 181066f differs from pull request most recent head 4687e02. Consider uploading reports for the commit 4687e02 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9736   +/-   ##
=======================================
  Coverage   86.09%   86.10%           
=======================================
  Files         327      328    +1     
  Lines       12579    12590   +11     
=======================================
+ Hits        10830    10840   +10     
- Misses       1749     1750    +1
Impacted Files Coverage Δ
lib/classes/Service.js 86.95% <50.00%> (-0.55%) ⬇️
lib/Serverless.js 71.84% <100.00%> (ø)
lib/aws/has-local-credentials.js 100.00% <100.00%> (ø)
lib/classes/PluginManager.js 95.15% <100.00%> (+0.01%) ⬆️
lib/cli/interactive-setup/index.js 88.88% <100.00%> (+0.88%) ⬆️
lib/cli/interactive-setup/utils.js 94.73% <100.00%> (+0.98%) ⬆️

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 4eb366b...4687e02. Read the comment docs.

@pgrzesik pgrzesik requested a review from medikoo July 13, 2021 10:03
medikoo
medikoo suggested changes Jul 14, 2021
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 that looks great. I've just proposed two naming improvements

lib/cli/interactive-setup/utils.js Outdated
return {
isInServiceContext: Boolean(serviceDir),
isLoggedIn: isAuthenticated(),
hasLocalCredentials: hasLocalCredentials(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's maybe name it hasLocalAwsCredentials

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, it's much better name 👍

lib/cli/interactive-setup/utils.js Outdated
resolvePreliminaryContext: ({ configuration, serviceDir }) => {
return {
isInServiceContext: Boolean(serviceDir),
isLoggedIn: isAuthenticated(),
Copy link
Contributor

Choose a reason for hiding this comment

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

And here maybe isLoggedIntoDashboard

(as here we're not specifically in dashboard context, it might be valuable to indicate all dashboard specific properties with "dashboard" token)

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
Copy link
Contributor Author

Thanks a lot for review, I'm also going to adjust the name of preliminaryContext to initialContext as it sounds more natural I believe

@medikoo
Copy link
Contributor

medikoo commented Jul 14, 2021 •
edited

I'm also going to adjust the name of preliminaryContext to initialContext as it sounds more natural I believe

Great call!

@pgrzesik pgrzesik force-pushed the resolve-preliminary-context-at-the-beginning-of-interactive branch from 7b20014 to 4687e02 Compare July 14, 2021 14:53
@pgrzesik pgrzesik requested a review from medikoo July 14, 2021 15:03
medikoo
medikoo approved these changes Jul 14, 2021
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 pgrzesik merged commit 560aee5 into master Jul 14, 2021
@pgrzesik pgrzesik deleted the resolve-preliminary-context-at-the-beginning-of-interactive branch July 14, 2021 16:48
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 cat/telemetry enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pgrzesik @medikoo