feat: Support Space CRD #328
Conversation
| - SpaceName | ||
| - DefaultSpaceSettings | ||
| - BatchDataCaptureConfig | ||
| - CodeRepositories |
There was a problem hiding this comment.
should this be part of separate PR? it seems unrelated
There was a problem hiding this comment.
so Space integrates with other SageMaker resources as part of SageMaker Unified Studio feature. This includes, App, Domain, Space, and UserProfile. All of these resources contain SpaceName, DefaultSapceSettings, and CodeRepositories. I've tested these features and they should be good to uncomment
| - CustomFileSystemConfig.FSxLustreFileSystemConfig | ||
| - CustomFileSystemConfig.S3FileSystemConfig |
There was a problem hiding this comment.
Old SDK version that was pinned to 1.32 had only CustomFileSystemConfig.EFSFileSystemConfig. Upgrade to SDK 1.39 added these two additional union fields. This field is used in Space, Domain, and UserSetting so they should be added.
| Tags: | ||
| compare: | ||
| is_ignored: true |
There was a problem hiding this comment.
For all sagemaker resources, we haven't implemented tag comparison/updates yet. I'm not sure if this has been done upstream from codegen already. But if it has, we'll probably require another PR that enable tag comparison across all resources.
…itexceeded from terminal
| - pipelines | ||
| - pipelineexecutions | ||
| - processingjobs | ||
| - spaces |
There was a problem hiding this comment.
Let's make sure this PR focuses on only bringing Spaces CRD
There was a problem hiding this comment.
Space resource is tightly integrated with Domain, UserProfile, and App resource. This is why the PR also contain some changes to these resources. If we dont make any change to those 3 resources to integrate with Space, the addition of Space resource is not going to be functional.
There was a problem hiding this comment.
Space is tightly coupled, resource won't function without userprofile changes
There was a problem hiding this comment.
Space is tightly coupled, resource won't function without domain changes
There was a problem hiding this comment.
s/test_studio.py/test_spaces.py/
There was a problem hiding this comment.
Test studio is more of an end to end test for SageMaker Unified Studio which consists of Domain, UserProfile, Space, and App
There was a problem hiding this comment.
It's hard to separate these resources into its own tests due to the coupling and dependency of these resources
There was a problem hiding this comment.
We've always introduced/tested a single resource at a time in ACK..this keeps things isolated and we can independently add more tests to the resources that need it more.
Here's an example of this in ec2: https://github.com/aws-controllers-k8s/ec2-controller/blob/main/test/e2e/tests/test_subnet.py#L56-L61
There was a problem hiding this comment.
I see, with the way ec2 controller is sharing resources across tests, this may be a bigger refactor than needed in this PR. Are we okay with merging this in and I can do a test refactor later on?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: michaelhtm, sirutBuasai The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.