Remote Dev Plugin Part 2 - Add basic remote commands#2546
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
98046df to
62e1d62
Compare
f6da57b to
2ff889f
Compare
| # If it already starts with SNOW_REMOTE_, use as-is | ||
| if name_input.startswith(f"{SERVICE_NAME_PREFIX}_"): | ||
| return name_input |
There was a problem hiding this comment.
The prefix check should be case-insensitive since service names are converted to uppercase later in the code. Consider modifying this to:
if name_input.upper().startswith(f"{SERVICE_NAME_PREFIX}_"):This ensures consistent handling regardless of the input case, matching the uppercase conversion applied in the else branch.
| # If it already starts with SNOW_REMOTE_, use as-is | |
| if name_input.startswith(f"{SERVICE_NAME_PREFIX}_"): | |
| return name_input | |
| # If it already starts with SNOW_REMOTE_, use as-is (case-insensitive) | |
| if name_input.upper().startswith(f"{SERVICE_NAME_PREFIX}_"): | |
| return name_input |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
+1, also consider resolving snowflake identifier rules (e.g. stripping quotes, etc)
2ff889f to
ecd97e7
Compare
8edd47b to
d8a093f
Compare
| stage: Optional[str] = typer.Option( | ||
| None, | ||
| "--stage", | ||
| help="Internal Snowflake stage to mount (e.g., @my_stage or @my_stage/folder). Maximum 5 stage volumes per service.", |
There was a problem hiding this comment.
What do you mean by Maximum 5 stage volumes? The Optional[str] type implies this is at most max 1?
There was a problem hiding this comment.
That's inherited from SPCS doc but it doesn't make sense here. I will remove it.
| image_tag: Optional[str] = typer.Option( | ||
| None, | ||
| "--image-tag", | ||
| help="Custom image tag to use for the remote development environment", |
There was a problem hiding this comment.
Is this limited to image tag or can it be full image path override?
There was a problem hiding this comment.
This is just for image tag for now. Given that we will have custom image, I will change it to allow the full image path
| result = self.execute_query("SELECT CURRENT_USER()").fetchone() | ||
| return result[0] if result else "unknown" |
There was a problem hiding this comment.
Will fetchone() throw if somehow the query result is empty?
There was a problem hiding this comment.
It will return None if the result is empty
| "Current status for %s: %s", service_name, current_status | ||
| ) | ||
| return True, current_status | ||
| else: |
There was a problem hiding this comment.
It shouldn't be. The check here is just to make sure the output we get is in the expected format.
| except Exception as e: | ||
| log.debug("Error checking service status for %s: %s", service_name, e) | ||
|
|
||
| return False, None |
There was a problem hiding this comment.
Should this be SERVICE_STATUS_UNKNOWN instead of None?
There was a problem hiding this comment.
I feel both could work here. I prefer to use None here to clear indicate no status information available.
| services = list_cursor.fetchall() | ||
|
|
||
| for service_row in services: | ||
| if len(service_row) > 0 and service_row[0] == service_name: |
There was a problem hiding this comment.
Does the Row have column names? May be better to use those instead of plain indexes
| # If it already starts with SNOW_REMOTE_, use as-is | ||
| if name_input.startswith(f"{SERVICE_NAME_PREFIX}_"): | ||
| return name_input |
There was a problem hiding this comment.
+1, also consider resolving snowflake identifier rules (e.g. stripping quotes, etc)
| # Assuming the row format is [name, port, public, protocol, host, url] | ||
| if len(row) >= 6 and row[0] == SERVER_UI_ENDPOINT_NAME: | ||
| return row[5] # Return the URL |
There was a problem hiding this comment.
Does the Row provide column names?
ecd97e7 to
c970827
Compare
d8a093f to
78b3491
Compare
c970827 to
d974a63
Compare
1bba2fd to
e1581b0
Compare
e1581b0 to
9f4cced
Compare
d974a63 to
440fe61
Compare
|
|
||
| # Use DESC SERVICE to get service information | ||
| # This returns exactly one row with service details, or none if service doesn't exist | ||
| desc_query = f"DESC SERVICE {service_name}" |
There was a problem hiding this comment.
Security Concern: There's a potential SQL injection vulnerability in this line. The service_name parameter is directly interpolated into the SQL query using an f-string without proper escaping or parameterization.
Consider using parameterized queries or proper SQL escaping instead. For example:
desc_query = "DESC SERVICE ?"
cursor = self.execute_query(desc_query, params=[service_name])This would ensure that any special characters in the service name are properly handled and prevent potential SQL injection attacks.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
9f4cced to
777824b
Compare
440fe61 to
5a2e34a
Compare
777824b to
564142b
Compare
5a2e34a to
3137229
Compare
564142b to
a794fed
Compare
3137229 to
2f06000
Compare

Pre-review checklist
Changes description
This PR adds several new basic commands to
remoteplugin, to create, manage, and connect to remote development environments running on SPCS Services. The plugin provides the following commands:snow remote start: Creates a new VS Code Server remote development environment or resumes an existing onesnow remote list: Lists all remote development environmentssnow remote stop: Suspends a remote development environmentsnow remote delete: Deletes a remote development environmentThe remote environments are deployed as SPCS Services with a web-based VS Code Server interface, allowing users to develop directly in Snowflake without local setup requirements. More details can be found in https://docs.google.com/document/d/1M0gV7soTy3dOadwgaUQrdKdkgR6YEUiwmBgMPlj_h5I/edit?usp=sharing