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

Migrate platform-sdk utils to serverless/utils tools #536

Merged

Conversation

pgrzesik
Copy link
Contributor

@pgrzesik pgrzesik commented Jan 29, 2021

This PR introduces the following changes:

  • Introduce @serverless/utils as a dependency
  • Bump @serverless/platform-client to latest version
  • Remove use of login from platform-sdk and replace it with login functionality already implemented in plugin
  • Replace getLoggedInUser, readConfigFile, writeConfigFile, refreshToken and logout from platform-sdk with utils equivalents

All of the above was tested with current testing suite + I've made a lot of "sanity" testing by hand locally.

Part of: #464

@codecov
Copy link

codecov bot commented Jan 29, 2021

Codecov Report

Merging #536 (bfb54ec) into master (48ac891) will increase coverage by 0.19%.
The diff coverage is 87.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #536      +/-   ##
==========================================
+ Coverage   48.84%   49.03%   +0.19%     
==========================================
  Files          90       90              
  Lines        2891     2896       +5     
==========================================
+ Hits         1412     1420       +8     
+ Misses       1479     1476       -3     
Impacted Files Coverage Δ
lib/login.js 31.25% <50.00%> (+9.02%) ⬆️
lib/logout.js 33.33% <50.00%> (+8.33%) ⬆️
lib/interactiveCli/utils.js 37.50% <66.66%> (+8.92%) ⬆️
lib/interactiveCli/index.js 96.96% <100.00%> (ø)
lib/interactiveCli/register.js 69.81% <100.00%> (+0.58%) ⬆️
lib/interactiveCli/set-app.js 91.26% <100.00%> (+0.21%) ⬆️
lib/isAuthenticated.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 48ac891...bfb54ec. Read the comment docs.

@pgrzesik
Copy link
Contributor Author

pgrzesik commented Feb 2, 2021

Sorry, I thought I've sent requests for review 😬

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.

Thank you @pgrzesik. Looks excellent! 👍

Just few questions:

  • Have integration tests were confirmed to pass?
  • Have you tested the interactive CLI onboarding with this changes?
  • How many more PR's do you think we need to fully ditch @serverless/platform-sdk from this package?

@pgrzesik
Copy link
Contributor Author

pgrzesik commented Feb 2, 2021

Thank you @medikoo - unfortunately, I didn't run the integration tests locally as I don't have access to organization org for tests, would you be able to invite me so I can double check before merging?

Have you tested the interactive CLI onboarding with this changes?

I believe I tested all cases multiple times and confirmed that it's working as intended, I didn't notice any regression or anything after my changes.

How many more PR's do you think we need to fully ditch @serverless/platform-sdk from this package?

I would like to close it with two more - I'm currently working on a PR that introduces a wrapper for getting ServerlessSDK instance initialized with token or access key retrieved from config that could be used in all places and after that, I'd like to migrate all existing methods (but that needs merging all related PRs in platform repository. Additionally, I will be moving SDK.Deployment from platform-sdk to enterprise-plugin and I believe it deserves it's own PR as well.

@pgrzesik
Copy link
Contributor Author

pgrzesik commented Feb 2, 2021

After talking with @medikoo on Slack I was able to successfully run integration tests locally both on master and this branch 💯

@eahefnawy
Copy link
Member

Looks good @pgrzesik 👍

@pgrzesik pgrzesik merged commit 4416651 into master Feb 4, 2021
@pgrzesik pgrzesik deleted the refactor-towards-removal-of-platform-sdk-use-utils-tools branch February 4, 2021 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants