-
Notifications
You must be signed in to change notification settings - Fork 881
feat: Add support for renaming workspaces #3409
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
Do you think we'll need to reuse the pg error checks? Should we have database pkg functions like: if database.IsUniqueConstraint(err, "column_name") {
// ...
} Just spitballing ideas. I think you also need this check on the |
I've been wondering if we should generate some Golang code like we do typescript code from our database schema. It might be a bit preemptive to go that far now, unless you think this error checking will be useful outside of just workspace and user names. If we do generate though, I'd like to expand our enum generation in Go. Currently sqlc generates Enums to Line 98 in 44423b3
|
My thinking is that the value is in ensuring our code is in sync with the schema. For instance, the unique index we're relying on here could be recreated with a different name at some point, and in that case the code would silently no longer catch the case. By generating the enums we ensure that the project fails to build unless names are in sync. Ultimately it'd be great if we strive for having all relevant parts generated into Go code (to ensure synchronization), but I don't think it needs to be an all or nothing approach. I.e. it'd be fine to generate a subset now, and more as the need arises. It didn't look like
(Not relevant for this PR, but) That looks annoying indeed. I think one of the libraries I mentioned in #3390 (comment) would support the custom |
This reverts commit 5ccd7a7.
952b5d3
to
f8dcf51
Compare
While #3386 is not fixed yet, I opted to add a large disclaimer to the rename command:
I think this should be sufficient for us to merge this functionality. |
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.
Frontend ✅
continue | ||
} | ||
if other.Name == arg.Name { | ||
return database.Workspace{}, &pq.Error{Code: "23505", Message: "duplicate key value violates unique constraint"} |
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.
TIL you can just return the correct SQL error codes 👍
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.
It's a bit janky 😅.. but at least the codes will never change.
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'm OK with this right now, bearing in mind that there is a really big open question about how to do this completely 100% safely. We should hide the command until this is 100% safe to do.
This fixes a TODO from #3409.
* feat: Generate DB unique constraints as enums This fixes a TODO from #3409.
WIP: This PR adds support for renaming workspaces.
For now, only the
docker
template has been updated, more to follow.TODO
Update all templates to use workspace IDs instead of namesWe will do this separately.Fixes #3000