- Notifications
You must be signed in to change notification settings - Fork1.1k
chore(coderd): remove the window option in open_in#16104
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
johnstcn commentedJan 13, 2025
Relates tocoder/terraform-provider-coder#297 |
johnstcn left a comment
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.
We will still have the database enum type'window'::workspace_app_open_in hanging around; I think we should drop this also.
| WINDOW=0; | ||
| SLIM_WINDOW=1; | ||
| TAB=2; | ||
| SLIM_WINDOW=0; |
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.
Let's keep theWINDOW member but mark it as deprecated.
As discussed, there's no way to remove an enum entry in Postgres so it's probably fine to just leave it lying around instead of the nonsense involved to get it removed properly.
defelmnq commentedJan 15, 2025
@dannykopping@johnstcn as per the comments and discussions we had, I :
|
dannykopping left a comment
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
johnstcn left a comment
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 was going to suggest updating the description of the enum'window'::workspace_app_open_in to mark it as deprecated, but it's not possible at the schema level as far as I can tell.
matifali commentedJan 15, 2025
@defelmnq have we also removed the related changes fromhttps://github.com/coder/terraform-provider-coder? |
defelmnq commentedJan 15, 2025
Not yet, I merge this one as soon as possible to unblock bruno - but will have to create also one indeed on the tf-provider-coder 👍 |
a160e8f intomainUh oh!
There was an error while loading.Please reload this page.
As we worked on adding a
open_inparameter for workspace_apps - we initially created three options :After further investigation,
windowshould not be used and has to be removed.ℹ️ I decided to remove the option instead of deprecating it as we've not created any release nor documented the feature. Can be discussed.