Skip to content

Conversation

@PotatoWKY
Copy link
Contributor

@PotatoWKY PotatoWKY commented Oct 9, 2025

Description

Added status checks for SageMaker space operations to prevent invalid actions.

Picture: When opening remote connection:

Screenshot 2025-10-08 at 4 20 19 PM

fix done

  1. The stopSpace function now validates space status before attempting to stop, showing appropriate warning messages for spaces already in Stopped/Stopping states or currently Starting.

  2. Also added status refresh before open remote connect

Testing Done

code debug


  • 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 October 9, 2025 00:14
@amazon-inspector-ohio
Copy link

⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done

@github-actions
Copy link

github-actions bot commented Oct 9, 2025

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.
  • 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.

@amazon-inspector-ohio
Copy link

✅ I finished the code review, and didn't find any security or code quality issues.

**Description**

Added status checks for SageMaker space operations to prevent invalid actions.

**fix done**

1. The stopSpace function now validates space status before attempting to stop, showing appropriate warning messages for spaces already in Stopped/Stopping states or currently Starting.

2. Also added status refresh before open remote connect

**Testing Done**

code debug
@PotatoWKY PotatoWKY changed the title feat(smus): Add status validation for space operations fix(smus): Add status validation for space operations Oct 9, 2025
void vscode.window.showWarningMessage('This space is already in Stopped/Stopping state')
return
} else if (node.getStatus() === 'Starting') {
void vscode.window.showWarningMessage('This space is starting, please stop it later')
Copy link
Contributor

@vpbhargav vpbhargav Oct 9, 2025

Choose a reason for hiding this comment

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

Can we change the text to be prescriptive? Similar to the actual API message? "Space {spaceName} is in Starting state. Wait until it is Running to attempt stop again." Same for the other one - Let's use the SpaceName instead of "this"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that would be better, let me find the spaceName variable and add it.

node: SagemakerSpaceNode | SagemakerUnifiedStudioSpaceNode,
ctx: vscode.ExtensionContext,
sageMakerClient?: SagemakerClient
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a lot for making the fix! Appreciate it!

return
}

await tryRefreshNode(node)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, this would also change the text on the Space node to reflect state after the API call right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the text (Stopped/Running) on the space would also be updated.

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.

Approving but please address comments before merge.

@laileni-aws laileni-aws merged commit 1c7792d into aws:master Oct 9, 2025
30 of 31 checks passed
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.

4 participants