Skip to content

Conversation

@nunogoncalves03
Copy link
Member

@nunogoncalves03 nunogoncalves03 commented Nov 22, 2024

We have recently updated the Stage Management API to add support for starter workspaces. Following this change, we would like to update the SingleStoreDB Python SDK to also accommodate starter workspaces in stage resources.

This PR introduces a basic StarterWorkspace object and modifies the Fusion SQL stage syntax. As discussed in Kanit's proposal, the IN GROUP syntax has been simplified to just IN. For example, the previous command SHOW STAGE FILES IN GROUP '{{ wg_name }}' has been updated to SHOW STAGE FILES IN '{{ wg_name }}'.

cc: @ricardoasmarques @kanitsharma

@nunogoncalves03
Copy link
Member Author

Should I create separate PRs for the Python SDK and Fusion SQL changes?

Copy link
Collaborator

@kesmit13 kesmit13 left a comment

Choose a reason for hiding this comment

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

I guess I'm a little confused by the changes here. It appears as though you are making the commands only work for starter workspaces and not for traditional ones. I'm saying this because of the change from get_workspace_group to get_starter_workspace. Is that the intent, or is get_starter_workspace intended to return either a starter workspace or a traditional one? If it returns either one, it shouldn't be named get_starter_workspace and the return type needs to include both StarterWorkspace and WorkspaceGroup.

We also probably need to create two SQL commands: one that keeps GROUP in the command and another than has it removed because we have already released the command with GROUP in it. I don't think we can just make GROUP optional in a single command because of abmiguities that would be in parsing.

@nunogoncalves03
Copy link
Member Author

nunogoncalves03 commented Nov 22, 2024

My bad, the get_starter_workspace function should be renamed to get_deployment, as it returns either a WorkspaceGroup or a StarterWorkspace.

Regarding the removal of the GROUP syntax, I misunderstood your comment on Kanit's proposal. I thought we were going to eliminate it rather than ensure backwards compatibility. That makes sense, I'll fix this.

@kesmit13
Copy link
Collaborator

I think it's fine to have duplicate commands for now. In fact, you can take what you have now and just copy the whole command and add a GROUP in the syntax so it works like it used to, but still works with both types of deployments. At some point, we can remove the old one.

Copy link
Collaborator

@kesmit13 kesmit13 left a comment

Choose a reason for hiding this comment

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

I noticed that the wrong string had been used in the Arguments sections of the commands for anything that has a multi-word name. The syntax in the Arguments and Remarks should use - rather than _. The - version is what shows up in the Fusion help system. I realize you were just using the form that was there before, but if you could change all of those in this PR, that would be great. After that, I think it's ready to go.

@nunogoncalves03 nunogoncalves03 force-pushed the ngoncalves/stage-starter-workspaces branch from 0368c84 to 25041ca Compare November 25, 2024 08:41
@nunogoncalves03 nunogoncalves03 changed the title Add support for starter workspaces within stage resources feat(stage): add support for starter workspaces Nov 25, 2024
@kesmit13 kesmit13 merged commit a3a0379 into main Nov 25, 2024
11 checks passed
@kesmit13 kesmit13 deleted the ngoncalves/stage-starter-workspaces branch November 25, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants