-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Allow updating workspace names #9686
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; I would use this review to add the slug as well.
@@ -1960,6 +1960,8 @@ components: | |||
email: | |||
type: string | |||
format: email | |||
name: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can also use the review to update the slug as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for pointing this out. I've made sure the slug is updated based on the new name. I assume we don't want (can't) let the user specifcy this, since the code we currenlty use anyway needs to "retry" until it's unique? Or would we want to let the API caller explicitly specify a slug and fail the request if that's already taken?
Could you also point me to a place the slug is used? It seems in URLs we're using the IDs of the workspace and not the slug?
I've changed the PR to actually pull the workspace renaming into a separate endpoint. The existing endpoint has a bit weird behavior that it requires a lot of the boolean flags to be set to udpate them. While I could make them optional it currently also has information like the notifications which are not required but if not specified will be updated to empty, which I assume is not what we want when calling this from cloud. Since I didn't want to break backwards compatibility now and potential usage of the API, I decided to pull it out into its own endpoint, that just handles the renaming of a workspace. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, it is true that going with a new endpoint sounds better, especially since it could break external integration on which we don't have control.
@benmoriceau Would you mind giving this PR a review approval still, if it looks good for you, so GitHub will be satisfied :) |
What
Part of #9524
This will allow workspace names to be updated via the API, which we'll call as part of updating a cloud workspace later to keep both names in sync.