Ditch @serverless/platform-sdk in favor of @serverless/platform-client

[medikoo]

@serverless/platform-sdk should no longer be used.

Note it's also used in @serverless/components and @serverless/safeguards-plugin. Ideally it should be also removed from them (at least from @serverless/components).

Implementation proposal:

In first phase, let's document all @serverless/platform-sdk methods that are used in those packages, and investigate weather we have reliable counterparts on @serverless/platform-client side

(do not implement any changes at this point)

[pgrzesik]

Below I've created a table with methods from platform-sdk that are used across enterprise-plugin, components and safeguards-plugin. This is still WIP, as I'm trying to wrap my head around how different pieces interact with each other. The methods that have ?? in front of them need more clarification, the ones with TODO I have yet to do (and understand). In the next comment, I'll post a few additional questions that could help me understand the bigger picture.

Method in platform-sdk	Method in platform-client	Used by	Notes
readConfigFile		components, enterprise-plugin	After discussion, these should be replaced by functionality from serverless/utils
writeConfigFile		components, enterprise-plugin	After discussion, these should be replaced by functionality from serverless/utils
getLoggedInUser		components, enterprise-plugin	I believe the logic for that functionality could be moved to either serverless/utils or separately to both components and enterprise-plugin as it heavily relies on accessing the local configuration file.
logout		components, enterprise-plugin	There's only a single use of logout from platform-sdk in enterprise-plugin and it adjusts config file + manipulates loggedInUser object. We will have to reimplement that function with config file manipulation based on utils from serverless/utils. In components the use looks the same.
login		enterprise-plugin	There is one use of login from platform-sdk left, but after following changes from: https://github.com/serverless/enterprise-plugin/pull/477/files it seems like it could be replaced by lib/login.js in enteprise-plugin
refreshToken		components, enterprise-plugin	After PR: https://github.com/serverlessinc/platform/pull/226/files gets merged, there will be a method to refresh a token in platform-client, sdk.session.refreshToken. That method, together will local file config manipulation (getting refresh token from it and writing refreshed token to it) will allow implementation of a similar method directly in enterprise-plugin and components
createAccessKeyForTenant		components, enterprise-plugin	After PR: https://github.com/serverlessinc/platform/pull/226/files gets merged, there will be a counterpart method for key creation in platform-client sdk.accessKeys.create
getAccessKeyForTenant		enterprise-plugin	After PR: https://github.com/serverlessinc/platform/pull/226/files gets merged, there will be a counterpart method for key creation in platform-client sdk.accessKeys.create That method, together with local file config manipulation will allow implementation of a similar method directly in enterprise-plugin, not in platform-client
getCredentials		enterprise-plugin	This method is not available in platform-sdk anymore, it was removed with: https://github.com/serverless/platform-sdk/commit/2c05b9567d40bd773eac5b972b04a8e2ce8f15da#diff-25a6634263c1b1f6fc4697a04e2b9904ea4b042a89af59dc93ec1f5d44848a26 It should be fixed in the enterprise-plugin as it currently, most likely, doesn't work unless I'm missing something
configureFetchDefaults		enterprise-plugin	Similar functionality is implemented as a part of platform-client, but agent is not injected to all requests, so that should be adjusted
getLogDestination		enterprise-plugin	I didn't find a counterpart method for this, so we would have to implement an API call ((${sdk.getDomain('core')}/malt/destinations/create)
removeLogDestination		enterprise-plugin	I didn't find a counterpart method for this, so we would have to implement an API call ((${sdk.getDomain('core')}/malt/destinations/delete)
getService		enterprise-plugin	I didn't find a counterpart method for this, so we would have to implement an API call (${sdk.getDomain('core')}/core/tenants/${orgName}/applications/${app.name}/services/${serviceName}
archiveService		enterprise-plugin	I didn't find a counterpart method for this, so we would have to implement an API call (${sdk.getDomain('core')}/core/tenants/${orgName}/applications/${app.name}/services/${serviceName}
getApps	listApps	enterprise-plugin	There is an equivalent method listApps
getApp		enterprise-plugin, safeguards-pluign	There is no direct counterpart available in platform-client, but implementation should be simple (api call to ${sdk.getDomain('core')}/core/tenants/${orgName}/applications/${app.name}
createApp	createApp	enterprise-plugin	There is an equivalent method but with caveat, in platform-client, payload includes description and deploymentProfiles properties which are not available in platform-sdk
getMetadata		enterprise-plugin	There's not a direct counterpart available in platform-client, but implementation should be simple (api call to ${sdk.getDomain('core')}/core/meta)
getStateVariable		enterprise-plugin	I didn't find a counterpart method for this, we would have to implement an API call ${sdk.getDomain('core')}/core/tenants/${tenant}/applications/${app}/services/${service}/stages/${stage}/regions/${region}/outputs We also need access to region
listTenants		components, enterprise-plugin	I didn't find a counterpart method for this, so we would have to implement an API call ((${sdk.getDomain('core')}/core/tenants?userName=${username})
register		enterprise-plugin	There's a corresponding method in platform-client called createOrg that calls the same endpoint, but does not support email/password parameters and is intended for creating organizations only. We would have to either expand it or implement a similar method that calls the same API but offers different signature
setDefaultDeploymentProfile		enterprise-plugin	There is no direct counterpart, so we would have to implement and API call (${sdk.getDomain('core')}/core/tenants/${tenant}/applications/${app.name})
createDeployProfile/createDeploymentProfile		enterprise-plugin	There is no direct counterpart, so we would have to implement and API call (${sdk.getDomain('core')}/core/tenants/${tenant}/deploymentProfiles)
getDeployProfiles		enterprise-plugin, safeguards-pluign	There's no direct counterpart available in platform-client, so we would have to implement an API call to ${sdk.getDomain('core')}/core/tenants/${orgName}/deploymentProfiles
getDeployProfile		enterprise-plugin	There's no direct counterpart available in platform-client, so we would have to implement an API call to ${sdk.getDomain('core')}/core/tenants/${orgName}/applications/${app.name}/profileValue
SDK.Deployment()		enterprise-plugin	I believe this should be moved to enterprise-plugin as it's the only service that consumes that class. It calls an endpoint that is not available in platform-client (${sdk.getDomain('core')}/core/tenants/${tenant}/applications/${app}/services/${service}/stages/${stage}/regions/${region}/deployments) and it might be required to implement it as a part of `platform-client. It also depends on getAccessKeyForTenant, so that refactoring will have to be performed first.
urls		components, enterprise-plugin	The config in platform-client is missing domains for local, preview and qa stages. Missing logDestinationUrl can be replaced in platform-client by using sdk.getDomain('malt'). I believe all auth0-related configurations doesn't have to be transferred to platform-client as they're not used by anything there. As for loginBrokerUrl - it is passed to sdk.login as part of config, but as it's statically obtained from platform-sdk, I believe it could be obtained by simply sdk.getDomain('login')/broker and removing dependency on passed in url.

[pgrzesik]

Some questions:

1. Is tenant equal to organization? My understanding so far suggests that the answer is 'yes' but I want to double-check
2. Is stageName from platform-client the same as stage in platform-sdk? What about platformStage?
3. What is an equivalent to serviceName from platform-sdk? Are we supposed to operate on IDs instead of names in context of platform-client?
4. Is the configuration on SDK supposed to completely replace local config-related functionalities (readConfigFile and writeConfigFile)?
5. What are the differences in login/logout flows? Are we supposed to migrate to the approach used in platform-client?
6. How are deploymentProfilesused?
7. Is region available in platform-client context?

@medikoo I would appreciate if you had a chance to look at them and provide more context 🙇

[medikoo]

@pgrzesik great questions, see replies below

Is tenant equal to organization? My understanding so far suggests that the answer is 'yes' but I want to double-check

Yes, it's old deprecated name (I've tried to ditch all occurrences, which we could ditch in @serverlress/enterprise-plugin with #344)

Is stageName from platform-client the same as stage in platform-sdk?

It corresponds to stage of an app in dashboard (so stage as we use in Framework and Components services)

What about platformStage?

It's a backend platform stage, not related in any way to stages to which users deploy their services.

We only have two active platform deployments (on prod and dev stages). No other stages on backend were created and are no intended to be created at this point

What is an equivalent to serviceName from platform-sdk? Are we supposed to operate on IDs instead of names in context of platform-client?

AFAIK slugs are used, which are combinations of service, app, org, stage and region. Check those utils: https://github.com/serverless/enterprise-plugin/blob/28cf1ecbfe24c69b2510a885ee732db7a1046797/lib/utils.js#L2-L7

@astuyve I believe will be able to put more light on that.

Is the configuration on SDK supposed to completely replace local config-related functionalities (readConfigFile and writeConfigFile)?

This should be replaced with usage of https://github.com/serverless/utils/blob/master/docs/config.md (idea is to handle user config read & write operation only through this util)

What are the differences in login/logout flows? Are we supposed to migrate to the approach used in platform-client

Login and logout already was migrated in context of this plugin (check: #477). It's only config read/write utils and dashboard urls that are retrieved from @serverless/platform-sdk, but that should be replaced with config util in @serverless/util and concerning urls, we need to confirm. If there's no such exposed by@serverless/platform-client then I believe we need to just hard-code them in plugin

How are deploymentProfiles used?

I take this is replaced by providers In platform client, best if we sync with @astuyve on that

Is region available in platform-client context?

It exists, and we can see it being a part of instanceSlug (instance represents specific service deployment)

[pgrzesik]

Thank you @medikoo for responding 🙇 It definitely made a few things much more clear for me. I've updated the table above with additional notes and I have a few extra questions:

1. Method getCredentials was removed as a part of: serverless/platform-sdk@2c05b95#diff-25a6634263c1b1f6fc4697a04e2b9904ea4b042a89af59dc93ec1f5d44848a26 but it's still being used in plugin: https://github.com/serverless/enterprise-plugin/blob/master/lib/credentials.js Am I missing something or it just doesn't work here?
2. How much of the userConfig-related logic should live in serverless/utils or rather, how specific the utils in serverless/utils should be? The best example would be getLoggedInUser and logout - these tools deal with userConfig file directly. Do they qualify for serverless/utils or they're components/plugin specific and they should just live in enterprise-plugin and components repositories separately? I'm trying to gauge how should we treat serverless/utils.
3. createAccessKeyForTenant, getAccessKeyForTenant and refreshToken utils - I'm having a hard time building a proper mental model on how these could fit into the platform-client - should we support that functionality as a part of platform-client or not? It depends on accessing userConfig in current form, but it could also take accessKey from SDK configuration in case of platform-client, but I'm not sure if we even need that. I feel like this is something that could be left out of platform-client, but I'm honestly not sure here.

[medikoo]

@pgrzesik great questions:

Method getCredentials was removed .. Am I missing something or it just doesn't work here?

Yes it can be just ditched out (as we discussed in the meeting). As I see in a history it was put behind the process.env.SLS_CLOUD_ACCESS (not documented and unknown to me env var) with refactor PR: #6. Looks as forgotten side effect that can simply be removed.

How much of the userConfig-related logic should live in serverless/utils or rather, how specific the utils in serverless/utils should be?

@serverless/utils is dedicated to store functionalities to be used by more than one of our open source packages (as serverlesss, @serverless/components, @serverless/enterprise-plugin). There's nothing more and nothing less about it.
I see it as part of our transitory state, as when we will reach the position where @serverless/components, @serverless/enterprise-plugin will be integrated into serverless package. It's likely that this package will become obsolete.

Now concerning user config. User config should be read and written only through @serverless/utils/config (it's important that we do not use any other utils to write or read config, as it implies a risk of overwriting some requested changes, or going out of sync).

If we need more high level utils (as getLoggedInUser) to be used by more than one OS package, then it's totally ok to place it in @serverless/utils, but if it's just for the sake of @serverless/enterprise-plugin it's probably best to put it there.

createAccessKeyForTenant, getAccessKeyForTenant and refreshToken utils - I'm having a hard time building a proper mental model on how these could fit into the platform-client

This is what we should probably discuss through with @ac360 and @astuyve in upcoming scheduled meeting, as my picture is also blurry.

---

Additional note, towards login usage. It was supposed to be fully replaced with #477 so we login just through @serverless/platform-client. You've pointed that old login is still used, and indeed it was left over by mistake as part of interactive setup in register, This can be considered as bug.

Still as you've noticed we also have there register logic, and I'm not sure if @serverless/platform-client exposes an API for that.

[corydorning]

you may already know this, but @serverless/platform-client uses a vulnerable version of axios (ssrf) that has been patched in axios 0.21.1 (more details: axios/axios#3369).

This impacts the serverless project due to the following dependency path:

serverless > @serverless/enterprise-plugin > @serverless/platform-client > axios

Just sharing since I didn't see it posted anywhere.

[pgrzesik]

After going through all responses here and my notes from a meeting last week, most of the things are now much clearer to me, I've updated the table with revised notes. I believe that in the next step, we can move to implementation, keeping in mind and trying to hit the following KRs (from https://github.com/serverlessinc/roadmap/issues/375):

• Finish adding in any missing and relevant Old Platform SDK logic
• Ensure the SDK does not read/write local files, like .serverlessrc, serverless.yml. Client-specific logic should be isolated in clients (e.g. local file manipulation, React hooks).
• Ensure the SDK can continue to work well in browsers, and be bundled minimally and successfully with webpack

but also keeping in mind all other KRs. Below is a short outline of how the implementation steps could look like:

Implementation

1. [DONE] Implement readConfigFile, writeConfigFile and getLoggedInUser equivalents in serverless/utils (Unify configuration file handling  utils#69)
2. [DONE] Implement logout, refreshToken equivalents in serverless/utils (Add account-related methods  utils#72)
3. [DONE] Use functionalities implemented above in serverless/utils and use them in enterprise-plugin. (Migrate platform-sdk utils to serverless/utils tools #536)
4. [DONE] Use functionalities implemented above in serverless/utils and use them in components. (refactor: Use utils instead of platform-sdk in login components#881)
5. [DONE] Replace login with functionality already available in enterprise-plugin. (Migrate platform-sdk utils to serverless/utils tools #536)
6. [DONE] Use createAccessKeyForTenant from platform-client in `enterprise-plugin
7. [DONE] Implement equivalent to getAccessKeyForTenant in enterprise-plugin and replace use of platform-sdk.
8. [DONE] Remove getCredentials usage as that method no longer exists in platform-sdk (Remove obsolete credentials.js module #530)
9. [DONE] Implement getLogDestination and removeLogDestination in platform-client (https://github.com/serverlessinc/platform/issues/251)
10. [DONE] Replace getLogDestination and removeLogDestination with platform-client equivalents in enterprise-plugin. (Partial migration from platform-sdk to platform-client #545)
11. [DONE] Implement getService in platform-client (https://github.com/serverlessinc/platform/issues/252)
12. [DONE] Use getService and archiveService equivalents from platform-client in `enterprise-plugin (Partial migration from platform-sdk to platform-client #545)
13. [DONE] Implement getApp in platform-client and move all existing methods into apps namespace (https://github.com/serverlessinc/platform/pull/277)
14. [DONE] Use platform-client app-related methods in enteprirse-plugin (Partial migration from platform-sdk to platform-client #545)
15. [DONE] Implement getMetadata in platform-client (https://github.com/serverlessinc/platform/pull/280)
16. [DONE] Use platform-client equivalent of getMetadata in enterprise-plugin (Partial migration from platform-sdk to platform-client #545)
17. [DONE] Use platform-client equivalent of listTenants in enterprise-plugin (Partial migration from platform-sdk to platform-client #545)
18. [DONE] Implement getStateVariable equivalent in platform-client (https://github.com/serverlessinc/platform/pull/281)
19. [DONE] Implement method for creating/saving a new deployment in platform-client (https://github.com/serverlessinc/platform/pull/281)
20. [DONE] Use platform-client equivalent of getStateVariable in enterprise-plugin and components (Partial migration from platform-sdk to platform-client #545)
21. [IN REVIEW] Move SDK.Deployment implementation to enterprise-plugin
22. [DONE] Remove dependency on explicitly passed loginBrokerUrl for login method in platform-client (https://github.com/serverlessinc/platform/pull/298)
23. [TO DO] Ensure that all functionality provided by configureFetchDefaults is present in platform-client
24. [DONE] Implement getDeployProfile, getDeployProfiles, createDeployProfile, setDefaultDeploymentProfile in platform-client and use it in enterprise-plugin and safeguards-plugin.

I'm a bit on the fence with implementation of refreshToken for example, as it relies on an API call from platform-client and will also depend on read/writeFileConfig from serverless/utils so it will have to be implemented twice in components and enterprise-plugin. I was thinking about putting shared implementation somewhere, but utils doesn't feel right (as it would require adding extra dependency on sdk in utils) - what are your thoughts on this one?

I'm open to feedback to the above, after I get a green light, I'll turn the rough list above into proper Github Issues. 💯

[medikoo]

Thanks @pgrzesik for the update. Great we have that finally coined! I have just few comments

Implement readConfigFile, writeConfigFile

I believe there's no need to implement those functions, as this functionality is already served by @serverless/utils/config (and we should not introduce any other utils which attempt to read or write the user config file)

I was thinking about putting shared implementation somewhere, but utils doesn't feel right (as it would require adding extra dependency on sdk in utils) - what are your thoughts on this one?

I think we shouldn't be afraid of that. Whole and only point of utils is to secure an ability to share functionalities among Framework, Components and Plugin. If that implies extra dependency in utils I wouldn't see it as a blocker.

[pgrzesik]

I believe there's no need to implement those functions, as this functionality is already served by @serverless/utils/config (and we should not introduce any other utils which attempt to read or write the user config file)

You're totally right, so part of this will be just confirming if it works as expected and adding an extra getLoggedInUser util 👍

I think we shouldn't be afraid of that. Whole and only point of utils is to secure an ability to share functionalities among Framework, Components and Plugin. If that implies extra dependency in utils I wouldn't see it as a blocker.

Here I was mostly worried about introducing a chain of dependencies which can be problematic to upgrade in some cases. If we introduce a dependency on platform-client in utils which is used in enterprise-plugin, then after upgrading platform-client we should upgrade utils and later upgrade both utils and platform-client in enterprise-plugin, unless we rely on a nested dependency on utils which I don't like. It also introduces the need to create an instance of SDK in utils (so passing all configuration), which is also less-than-ideal.

What do you think?

[medikoo]

If we introduce a dependency on platform-client in utils which is used in enterprise-plugin, then after upgrading platform-client we should upgrade utils and later upgrade both utils and platform-client in enterprise-plugin

Yes, but I don't think it's that problematic. I still would prefer to reuse same utility instead of maintaining two copies of it, in two different packages.

Aside of a burden of bumping versions and releasing two packages do you envision any other issues it may create?

Also it's a temporary state, we're going into direction where @serverless/components, @serverless/enterprise-plugin and @serverless/utils will be archived and their functionalities will be a part of serverless package (but sure, it'll definitely take a long time until we'll get there)

[pgrzesik]

At this point I don't see any other issues that this situation might create other than this minor inconvenience during updates. 👍

[pgrzesik]

I'm coming back to this as starting the migration is next on our priority list and there's one topic that I'd like to resolve to ensure that it's going to be as smooth as possible. What is the standard way to test out new SDK (platform-client) against services that are still not migrated to https://github.com/serverlessinc/platform but instead are still a part of https://github.com/serverless/enterprise-dashboard? I see that as of right now, the only tests for platform-client are done via tests/integration.test.js that issue requests to sp-* services from platform. Is there an environment or existing tests where I could validate newly implemented platform-client methods?

cc @ac360 @astuyve

[medikoo]

What is the standard way to test out new SDK (platform-client) against services that are still not migrated to https://github.com/serverlessinc/platform but instead are still a part of https://github.com/serverless/enterprise-dashboard?

I think running integration tests of @serverless/enterprise-plugin should cover that. Check https://github.com/serverless/enterprise-plugin/tree/master/integration-testing and I believe @astuyve or @scouredimage can ensure you have all needed access

[pgrzesik]

Thanks @medikoo - if I understand correctly, it seems like all these type of tests would have to run via CLI which is good, but I was also looking for a potentially similar experience as in integration.test.js from platform repository, where SDK directly issues requests to the backend instances. Is there a similar dev instance of enterprise-dashboard that I can test out SDK against?

[medikoo]

where SDK directly issues requests to the backend instances. Is there a similar dev instance of enterprise-dashboard that I can test out SDK against?

I believe there's not, as that qualifies as integration tests for @serverless/platform-sdk and we can confirm it has none

[medikoo]

Anyway, why exactly you want to test against old dashboard? AFAIK point of this migration is to move things so everything works against new platform

[pgrzesik]

Anyway, why exactly you want to test against old dashboard? AFAIK point of this migration is to move things so everything works against new platform

@medikoo Unfortunately, not all backend functionality is migrated to new platform and platform-client still calls "old" backend services. One of the examples of new methods that need to be implemented is getLogDestination and removeLogDestination which calls "malt" service API from enterprise-dashboard.

[medikoo]

@medikoo Unfortunately, not all backend functionality is migrated to new platform and platform-client still calls "old" backend services.

I see. In such case best if you confirm with @ac360 and @astuyve on best path.

I believe that either we should improve platform and migrate fully platform-client to new platform. Or introduce temporary integration tests against old dashboard in a platform.

[pgrzesik]

Hello team, there are a few open PRs that I could use a little bit of feedback on, I also have a few questions that came up during implementation that I was hoping maybe you can answer.

The PRs in question:

• https://github.com/serverlessinc/platform/pull/277
• https://github.com/serverlessinc/platform/pull/280
• https://github.com/serverlessinc/platform/pull/282
• https://github.com/serverlessinc/platform/pull/278
• https://github.com/serverlessinc/platform/pull/281

Questions:

1. Challenging testing of services-related methods and lack of integration tests for them. From what I've noticed, the only way we actually create a new service is implicit via new deployments. Is that correct?
2. Is it expected that service-related APIs are only accessible with org-specific accessKey and not with idToken? That's what I've noticed from my testing today
3. Validation - In some PR's I've followed the pattern to validation that was introduced by @ac360 for accessKeys where each param is validated separately, but I'm not sure about that approach as it's error prone + so-so from UX perspective as imo validation should return you all missing properties, not only the first one and so on. We could e.g. use joi or ajv for such validation. What do you think?
4. Providing params as separate arguments vs passing an object with keys - this was brought up by @medikoo and I agree that current approach might be error prone as it's easy to mix up the ordering of parameters. In my PRs I've decided to stick to the current convention but I believe it would be worth considering switching to object-based approach moving forward. What do you think? Bonus of that would be making it easier to integrate with schema libraries mentioned in 3.
5. registerUser - this method is a tough one - I've implemented it as a direct equivalent to register from platform-sdk, but I've noticed that we already have two quite similar methods (one of them is createUserAndOrg that could potentially be adjusted. Is there a reason why these methods do not support providing user/password?
6. What about all methods related to deploymentProfiles? I know we're migrating off of them, but at the moment they seem to be required. Should I wait with these so we can drop them altogether? When would be the appropriate time to do that?

@medikoo @ac360 @astuyve I would really appreciate reviews + feedback on the above questions, thanks in advance 🙇

[medikoo]

but I'm not sure about that approach as it's error prone + so-so from UX perspective as imo validation should return you all missing properties, not only the first one and so on. We could e.g. use joi or ajv for such validation.

I think using joi or ajv for internal functions args validation would rather be overkill (those utils are more suited for validating complex configuration structures as e.g. serverless.yml)

If it's about validation within our internal functions (so private in context of Framework or Components users), then I believe throwing as soon as first error is discovered is ok. Those errors signal programmer error, and ideally (assuming good tests coverage) should happen only in development phase and be picked before merging to master (they can be seen just as debugging aid at development stage)

Still we may improve the validation by not only checking existence, but also by confirming on content type has nice dedicated utils which we already use in some places in Framework.
Also there's accumulated input validation provided by type/ensure, although at this point it doesn't support going inside object properties (but we can extend it to follow that as well)

I believe it would be worth considering switching to object-based approach moving forward.

+1, as we're on that. I would push that further, but confirm with @ac360 and @astuyve

[pgrzesik]

I think using joi or ajv for internal functions args validation would rather be overkill (those utils are more suited for validating complex configuration structures as e.g. serverless.yml)

If it's about validation within our internal functions (so private in context of Framework or Components users), then I believe throwing as soon as first error is discovered is ok. Those errors signal programmer error, and ideally (assuming good tests coverage) should happen only in development phase and be picked before merging to master (they can be seen just as debugging aid at development stage)

I was thinking about it more from external users' perspective, as platform-client is supposed to be not only our internal SDK, where in such case I totally agree that using more sophisticated client-side validation is an overkill, but also intended to be used by 3rd party users to interact with our platform and potentially automate some of the operations - but maybe I'm overthinking it too much at this point and even in such case, we won't need machinery such as ajv or joi. 👍 I like the suggestion of using type library 👍

[medikoo]

For costumer facing API's, I think type library will fit the use case best (It's dedicated for simple input args validation). It's capable of providing very informative error feedback (stating which argument is not right and why)

[pgrzesik]

Discussed during the Framework call (02/02/2021):

1. Validation in platform-client methods - on call we discussed that the best time to introduce "proper" validation will be after we have common guidelines on how we'd like to do that across our projects, which means that it won't be a part of current SDK migration.
2. When it comes to testing services(or anything really with integration tests), we should strive to replicate real-life workflows as much as possible. For example, in the case of testing service-related utils, we should couple it with creating a deployment (which will create a service as well).
3. Refactoring method params to a single object - during the call we discussed and agreed that this is the best approach moving forward and if we're migrating a lot of methods anyway, it's the best time to also migrate params to object to avoid two big releases introducing breaking changes that require a lot of adjustments in clients.

Pending questions:

1. What about all methods related to deploymentProfiles? I know we're migrating off of them, but at the moment they seem to be required. Should we wait with these so we can drop them from enterprise-plugin? When would be the appropriate time to do that?
2. registerUser - this method is a tough one - I've implemented it as a direct equivalent to register from platform-sdk, but I've noticed that we already have two quite similar methods (one of them is createUserAndOrg that could potentially be adjusted. Is there a reason why these methods do not support providing user/password?
3. Stages - in platform-sdk's config.js, we have domains configured for local, preview, dev , qa and prod stages. In platform-client, we only have local, dev, and prod. Do we still use qa and preview environments and should platform-client support them as well?
4. Login - currently in platform-client we're relying on loginBrokerUrl being passed as a part of config - is there a reason to why we're doing it this way instead of resolving loginBrokerUrl internally in a similar fashion to how all other URLs are resolved via sdk.getDomain? Right now, it's obtained from platform-sdk 's urls. I'd love to change it which will remove the dependency on platform-sdkbut I'm not sure about potential implications.
5. Migration of methods to namespaces. One of the objectives is to move all "standalone" methods to their specific namespaces (see webhooks or events) - it's yet another big breaking change - should it also be done as a part of this migration? I feel like it should help avoid yet another big breaking change + migration in the not-too-distant future.

Next steps for me (high level):

1. Wrap up PRs in enterprise-plugin and components with utils introduction for configFile-related methods
2. Clean up currently opened PRs + introduce a missing method for deployment + missing integration tests
3. Migrate methods to accept object instead of separate params
4. Wrap up all the above changes in platform-client and release a new major with breaking changes
5. Drop platform-sdk dependency in all project and replace with corresponding methods from platform-client

@ac360 @eahefnawy @astuyve I would love to hear you input on the above, thanks in advance 🙇

[astuyve]

1. Deployment profiles. Right now the vast majority of our users have been migrated off of the service, but a handful have not yet upgraded for varying reasons. Therefore we shouldn't backport them to the platform-client, but we can't remove that functionality entirely yet.
2. unclear on why createUserAndOrg exists or where it's used. Unless someone else can speak to its purpose, it should be deprecated.
3. QA and preview environments are deprecated, their uses now exist inside the dev environment
4. Seems like a safe change
5. Unclear, but that is the direction we want to move.

[medikoo]

When it comes to testing services(or anything really with integration tests), we should strive to replicate real-life workflows as much as possible.

I'll add, that I believe we should focus on (1) testing functionality that's implemented in given package (e.g. in platform-client we should cover testing implementation that it's in platform-client but not in enterprise-plugin), and (2) test it with same approach as one that is used by package dependents. Still e.g. if dependent feeds function (which we test) with some large structure that lacks any semantics in context of given function. I would simplify testing, by constructing simpler input, as in this context we do not have to strive to replicate exactly same large, complex (but meaningless to tested function) input

[pgrzesik]

Thanks @astuyve and @medikoo 🙇

I've grepped around for createOrg and createUserAndOrg methods from platform-client and here's what I've found:

• createOrg - looks like it's not really used at all, I've found that in enterprise-dashboard it's done with direct API calls, not via SDK (https://github.com/serverless/enterprise-dashboard/blob/e10021dc70e57e2658c3a5d8839f309484f110e3/frontend/src/utils/api/tenants.js#L22). Similarily in platform-dashboard - it's doing direct API calls (https://github.com/serverlessinc/platform-dashboard/blob/e5efe2e9e4db39120f8526e205e6c57250acbb29/src/team/components/CreateNewOrganization.js#L108)
• createUserAndOrg - I've found one place in platform-dashboard where it's being used: https://github.com/serverlessinc/platform-dashboard/blob/e5efe2e9e4db39120f8526e205e6c57250acbb29/src/auth/utils.js#L286

@stevewillard - do you happen to know why in the createUserAndOrg user is not created with email and password? As for createOrg method - I'm assuming it's going to eventually replace the direct API calls that I've mentioned above?

[stevewillard]

For createOrg, yeah if there's a platform-client SDK method for it, then I can swap over to using that instead of the direct endpoint.

For createUserAndOrg, this method is only used when you register a new account in the dashboard. When you register, you either socially auth (Google or GitHub), or provide an email and password. Doing so creates a user in auth0. Once you create an identity in auth0, you have to choose a username (or, more accurately, account name). This creates a serverless user and org. The auth0 id is saved as part of the serverless user record - that's how they are linked. But, that's why only username is passed in.

[pgrzesik]

Thank you @stevewillard for clarification, it all makes sense to me now 👍

[pgrzesik]

A small update from me.

PRs that are ready for review and safe to merge to current master (no breaking changes):

• Removing requirement to pass loginBrokerUrl to login https://github.com/serverlessinc/platform/pull/298
• Add metadata.get method https://github.com/serverlessinc/platform/pull/280
• Add services and deployment-related methods: https://github.com/serverlessinc/platform/pull/281/files

PRs that are ready for review, have their descriptions update and include breaking changes = are not safe to merge to master:

• Move organizations-related method into namespace and introduce param-object: https://github.com/serverlessinc/platform/pull/282
• Move app-related methods into namespace + implement get method and introduce param-object: https://github.com/serverlessinc/platform/pull/277
• Adjust logDestination methods to use object params: https://github.com/serverlessinc/platform/pull/297

Having these in should allow for a full migration from platform-sdk in enterprise-plugin (excluding any methods related to deploymentProfiles for now, all of them would still depend on platform-sdk 😞 ). The next steps here would be to:

1. Merge all PRs that are safe to merge without introducing a breaking change
2. Create a separate branch that will gather all "breaking" changes for v4 major release of platform-client. As a part of that, prepare all methods that will be broken and needs to be fixed during upgrade to v4 of platform-client

Questions to answer before releasing v4:

1. Migration of methods to namespaces. One of the objectives is to move all "standalone" methods to their specific namespaces (see webhooks or events) - it's yet another big breaking change - should it also be done as a part of this migration? I feel like it should help avoid yet another big breaking change + migration in the not-too-distant future, but it's heavily adding to the scope.
2. I know it was already discussed, but it's adding quite heavily to the scope - refactoring methods to params - should it be done as a part of this migration? If the answer is yes, I think we should both migrate params as well as all methods to namespaces. It will be much bigger scope, but doing one without the other doesn't make sense for me.

I'd love to hear your opinion on that + getting fresh reviews on listed PRs. Thanks in advance 🙇

[medikoo]

@pgrzesik great feedback, all looks great to me. I have just one question

Having these in should allow for a full migration from platform-sdk in enterprise-plugin (excluding any methods related to deploymentProfiles for now, all of them would still depend on platform-sdk 😞 )

What is our plan to address deploymentProfiles. Do we plan to simply remove this functionality?

[pgrzesik]

Thanks @medikoo 🙇

As @astuyve answered about deploymentProfiles:

Deployment profiles. Right now the vast majority of our users have been migrated off of the service, but a handful have not yet upgraded for varying reasons. Therefore we shouldn't backport them to the platform-client, but we can't remove that functionality entirely yet.

It seems like we cannot remove that functionality yet, but the plan is to remove it at some point in the future.

[pgrzesik]

Another update (hopefully the last one regarding the approach to migration 😅):

During the Framework call yesterday, we established that in order to make the SDK migration smoother, we will aim to not introduce ANY breaking changes at this point, which means that we will keep old, non-namespaced methods as deprecated if a namespaced equivalent exists, we also don't migrate any existing methods to object-param at this point. We still want to do it, but at a later point in time. For now, we will make sure that everything is backwards compatible. Additionally, we agreed that in order to not block migration, we will port all deploymentProfiles-related methods to platform-client (cc @astuyve ).

Below you can find a list of PRs that are ready for review - once these get merged, we will have all missing functionality available in the platform-client and the next step will be to retire use of platform-sdk in enterprise-plugin, components and safeguards-plugin.

PRs:

• Removing requirement to pass loginBrokerUrl to login: https://github.com/serverlessinc/platform/pull/298
• Add metadata.get method: https://github.com/serverlessinc/platform/pull/280
• Add services and deployments-related methods: https://github.com/serverlessinc/platform/pull/281
• Move organizations-related method into namespace: https://github.com/serverlessinc/platform/pull/282/files
• Move app-related methods into namespace + implement get method and introduce param-object: https://github.com/serverlessinc/platform/pull/277
• (This is technically breaking, but no one is using that API yet so it should be fine) Adjust logDestination methods to use object params: https://github.com/serverlessinc/platform/pull/297
• Add deploymentProfiles-related methods: https://github.com/serverlessinc/platform/pull/301

Reviews to the above will be much appreciated and will greatly speed up the migration process 🙇

[medikoo]

@pgrzesik great thanks for this outstanding work.

I've reviewed all those to which I was assigned (note that e.g. https://github.com/serverlessinc/platform/pull/282 has no reviewers assigned at this point, is it not finished yet?)

I believe best if you ping @astuyve directly on Slack, so he's fully aware with what it's expected from him. (@ac360 is on the road, so I believe this week we won't see reviews from him)

[pgrzesik]

Thank you @medikoo 🙇 As for https://github.com/serverlessinc/platform/pull/282 - It was an omission on my part as I didn't request reviews via Github there - it's ready for review as well.

[pgrzesik]

As a research for next steps, I have a question n the context of the following acceptance criteria:

• Ensure the SDK does not read/write local files, like .serverlessrc, serverless.yml. Client-specific logic should be isolated in clients (e.g. local file manipulation, React hooks). Please note that zipping is a necessary SDK utility at this time.

I also have an additional question about the use of fs in registry part of the new platform-client. From what I see, only components depend on that part of the API. Is there a reason that this part of SDK depends on interaction with local files via fs module? Could the file manipulation be moved to @serverless/components or e.g. @serverless/utils safely if it's being used by other packages as well?

@medikoo @eahefnawy @ac360 I would love to hear your opinion on the above

[pgrzesik]

Today we've released the @serverless/enterprise-plugin in version 4.5.0 which no longer depends on platform-sdk - I'm closing this issue and I'll provide a longer recap/next steps in parent issue: https://github.com/serverlessinc/company/issues/375