- Notifications
You must be signed in to change notification settings - Fork907
feat: persist app groups in the database#17977
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
Uh oh!
There was an error while loading.Please reload this page.
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.
All nits, won't block an approve
Uh oh!
There was an error while loading.Please reload this page.
| @@ -0,0 +1 @@ | |||
| alter table workspace_apps add column display_group text; | |||
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.
the other text fields usecharacter varying(65534),, but I thinktext is better 👍. Should we limit the size of the display_group??
coder/coderd/database/dump.sql
Lines 1979 to 1980 in4931c61
| commandcharacter varying(65534), | |
| urlcharacter varying(65534), |
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 definitely should, as it's much easier to increase an existing limit than it is to enforce a new one. If thedisplay_group is going to need to fit inside a similar size UI element toworkspace_apps.display_name character varying(64), it may make sense to copy that limit over.
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.
yes, but it should be validated by the terraform provider, or you won't notice the issue until you build a workspace. we should surface any validation issues at plan time.
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.
Addressed bycoder/terraform-provider-coder#407
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -0,0 +1 @@ | |||
| alter table workspace_apps add column display_group text; | |||
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 definitely should, as it's much easier to increase an existing limit than it is to enforce a new one. If thedisplay_group is going to need to fit inside a similar size UI element toworkspace_apps.display_name character varying(64), it may make sense to copy that limit over.
Uh oh!
There was an error while loading.Please reload this page.
What is an App Group? The PR has no description or link to an issue. Theterraform provider text for the "group" field just says "The name of a group that this app belongs to." |
9fc3329 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Part of#8237
Apps with the same agent and group name will be grouped together in the UI. On workspace build, we need to persist this value so that we can create these groupings later.