Skip to content
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

Allow updating workspace names #9686

Merged
merged 7 commits into from
Jan 31, 2022
Merged

Conversation

timroes
Copy link
Collaborator

@timroes timroes commented Jan 21, 2022

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.

@timroes timroes added the area/platform issues related to the platform label Jan 21, 2022
@github-actions github-actions bot added area/api Related to the api area/documentation Improvements or additions to documentation area/server labels Jan 21, 2022
@timroes timroes temporarily deployed to more-secrets January 21, 2022 09:26 Inactive
@timroes timroes temporarily deployed to more-secrets January 21, 2022 09:37 Inactive
@timroes timroes temporarily deployed to more-secrets January 21, 2022 10:36 Inactive
@timroes timroes temporarily deployed to more-secrets January 21, 2022 11:00 Inactive
@timroes timroes temporarily deployed to more-secrets January 21, 2022 11:29 Inactive
@timroes timroes temporarily deployed to more-secrets January 21, 2022 13:40 Inactive
@timroes timroes temporarily deployed to more-secrets January 21, 2022 14:00 Inactive
Copy link
Contributor

@benmoriceau benmoriceau left a 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:
Copy link
Contributor

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.

Copy link
Collaborator Author

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?

@timroes timroes temporarily deployed to more-secrets January 24, 2022 09:52 Inactive
@timroes timroes temporarily deployed to more-secrets January 25, 2022 10:21 Inactive
@timroes timroes temporarily deployed to more-secrets January 25, 2022 15:12 Inactive
@timroes
Copy link
Collaborator Author

timroes commented Jan 26, 2022

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.

@timroes timroes temporarily deployed to more-secrets January 26, 2022 09:34 Inactive
Copy link
Contributor

@benmoriceau benmoriceau left a 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.

@timroes
Copy link
Collaborator Author

timroes commented Jan 27, 2022

@benmoriceau Would you mind giving this PR a review approval still, if it looks good for you, so GitHub will be satisfied :)

@benmoriceau benmoriceau self-requested a review January 27, 2022 16:03
@timroes timroes merged commit 8f22595 into master Jan 31, 2022
@timroes timroes deleted the tim/server/workspace-update-name branch January 31, 2022 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants