Skip to content

feat(smus): Upgrade sdk v2 to sdk v3 - DataZoneCustomClient #8411

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
Open
PotatoWKY wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(smus): Upgrade sdk v2 to sdk v3 - DataZoneCustomClient #8411

PotatoWKY wants to merge 1 commit into from
+1,208 −1,051

Conversation

@PotatoWKY
Copy link
Contributor

@PotatoWKY PotatoWKY commented Dec 11, 2025

Summary

This PR migrates the DataZone custom API client from the deprecated AWS SDK v2 generator pattern to standalone AWS SDK v3-compatible packages. This migration is required as part of the broader AWS Toolkit repository migration from SDK v2 to v3.

The AWS Toolkit VSCode repository is deprecating the centralized generateServiceClient.ts script that generates TypeScript clients from service JSON definitions. The SageMaker Unified Studio (SMUS) team currently uses this v2 generator for DataZone Custom API.

Solution

  • Created standalone package @amzn/datazone-custom-client with SDK v3 patterns
  • Updated DataZoneCustomClientHelper to use SDK v3 client initialization and Command pattern
  • Migrated from client.methodName().promise() to client.send(new Command()) pattern
  • Updated credential provider to use SDK v3 async credential provider function
  • Updated imports to use new standalone package types and commands
  • Updated test files to mock SDK v3 send() method instead of v2 .promise() pattern
  • Removed dependency for DataZone Custom API on the deprecated generateServiceClient.ts script
  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@PotatoWKY PotatoWKY requested a review from a team as a code owner December 11, 2025 01:22
@PotatoWKY PotatoWKY force-pushed the sdk-upgrade-datazone branch from 2890cdb to 0f08f21 Compare December 11, 2025 01:23
@PotatoWKY PotatoWKY changed the title Upgrade sdk v2 to sdk v3 - DataZoneCustomClient feat(smus): Upgrade sdk v2 to sdk v3 - DataZoneCustomClient Dec 11, 2025
@PotatoWKY PotatoWKY closed this Dec 11, 2025
@PotatoWKY PotatoWKY reopened this Dec 11, 2025
@github-actions
Copy link

github-actions bot commented Dec 11, 2025

  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

vpbhargav
vpbhargav reviewed Dec 11, 2025
View reviewed changes
Copy link
Contributor

@vpbhargav vpbhargav left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes! How are we testing and ensuring there is no regression?

packages/core/src/sagemakerunifiedstudio/shared/client/datazoneCustomClientHelper.ts
Comment on lines +107 to +115
const awsCredentialProvider = async () => {
const credentials = await this.credentialProvider.getCredentials()
return {
accessKeyId: credentials.accessKeyId,
secretAccessKey: credentials.secretAccessKey,
sessionToken: credentials.sessionToken,
expiration: credentials.expiration,
}
}
Copy link
Contributor

@vpbhargav vpbhargav Dec 11, 2025

Choose a reason for hiding this comment

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

This wrapper needed? Unable to pass this.credentialProvider directly to the clientConfig? Seems like it but just want to confirm.

Copy link
Contributor Author

@PotatoWKY PotatoWKY Dec 11, 2025

Choose a reason for hiding this comment

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

Yeah it is needed. I am not Typescript pro, but here is my understanding:

  • passing this.credentialProvider would only pass the method,
  • with the wrapper it also passes the instance

we would need the correct instance to get the credentials
Inspired by @liuzulin:
https://github.com/aws/aws-toolkit-vscode/blame/20a5e850bc483e0d649eddf1880014e2873ca565/packages/core/src/sagemakerunifiedstudio/shared/client/s3Client.ts#L159

packages/core/src/test/sagemakerunifiedstudio/shared/client/datazoneCustomClientHelper.test.ts Outdated
listDomains: sinon.stub().returns({
promise: () => Promise.resolve(mockResponse),
}),
send: sinon.stub().resolves(mockResponse),
Copy link
Contributor

@vpbhargav vpbhargav Dec 11, 2025

Choose a reason for hiding this comment

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

Are we able to be specific with which method is being called for this sub as well? listDomains in this case?

packages/core/src/test/sagemakerunifiedstudio/shared/client/datazoneCustomClientHelper.test.ts
assert.deepStrictEqual(mockDataZoneClient.getDomain.firstCall.args[0], {
identifier: mockDomainId,
})
assert.ok(mockDataZoneClient.send.calledOnce)
Copy link
Contributor

@vpbhargav vpbhargav Dec 11, 2025

Choose a reason for hiding this comment

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

These assertion are not the same. We were validating input params earlier and now we are just validating that some send call is made. Are we able to maintain the old assertions?

Same for other places in test too.

Copy link
Contributor Author

@PotatoWKY PotatoWKY Dec 11, 2025

Choose a reason for hiding this comment

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

Yeah you are right, the verifications are over simplified. let me add some more detailed validation

packages/core/src/test/sagemakerunifiedstudio/shared/client/datazoneCustomClientHelper.test.ts
domainId: mockDomainId,
id: 'gp_profile1',
status: 'ACTIVATED',
status: 'ASSIGNED',
Copy link
Contributor

@vpbhargav vpbhargav Dec 11, 2025

Choose a reason for hiding this comment

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

Why did this need to change? Previous test was wrong?

Copy link
Contributor Author

@PotatoWKY PotatoWKY Dec 11, 2025

Choose a reason for hiding this comment

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

Yeah it was wrong, it would be either UNASSIGNED or ASSIGNED

@PotatoWKY
Copy link
Contributor Author

PotatoWKY commented Dec 11, 2025

Thanks for making these changes! How are we testing and ensuring there is no regression?

I did sanity test for each of the 3 SDK upgrade PRs, I think we would also want to let the team test the prerelease vsix after the 3 PRs are merged.

@PotatoWKY PotatoWKY force-pushed the sdk-upgrade-datazone branch from 0f08f21 to 8cc99e4 Compare December 12, 2025 00:48
@PotatoWKY PotatoWKY force-pushed the sdk-upgrade-datazone branch from 8cc99e4 to 024ac3d Compare December 13, 2025 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@vpbhargav vpbhargav vpbhargav left review comments

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PotatoWKY @vpbhargav