NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

feat: allow configuring display apps from template#9100

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

Jump to bottom

Merged

sreya merged 7 commits intomainfromjon/defaultapps

Aug 30, 2023

Merged

feat: allow configuring display apps from template#9100

sreya merged 7 commits intomainfromjon/defaultapps

Aug 30, 2023

Conversation

Copy link

Collaborator

sreya commentedAug 15, 2023•
edited
Loading

This PR implements the control plane portion of thedisplay_apps field.

The default behavior for the field is to opt-in to all apps (except vscode_insiders).
To opt-out of all you must specifyfalse to all fields.
Otherwise you only get the apps that you explicitly set totrue.

This PR implements both the backend and frontend portions of the feature. Notably on the frontend the dropdown forVSCode Desktop ceases to be a dropdown if only one ofVSCode Desktop orVSCode Insiders is enabled.

Some screenshots:

displays_apps {}

display_apps {  vscode = false  vscode_insiders = true}

display_apps {  vscode_insiders = true}

fixes#8033

github-actionsbot assignedsreya

Aug 15, 2023

matifali reviewed

Aug 15, 2023

View reviewed changes

codersdk/workspaceagents.go OutdatedShow resolvedHide resolved

sreya force-pushed thejon/defaultapps branch frome918531 to00606c9Compare

August 18, 2023 21:08

sreya marked this pull request as ready for review

August 18, 2023 22:30

sreya changed the title~~feat: add default_apps field to agent~~feat: allow disabling default apps from template

Aug 18, 2023

sreya force-pushed thejon/defaultapps branch fromfee5bd3 toe0e0837Compare

August 24, 2023 19:06

sreya changed the title~~feat: allow disabling default apps from template~~feat: allow configuring display apps from template

Aug 24, 2023

sreya force-pushed thejon/defaultapps branch 9 times, most recently from3ef47ec tod6f467cCompare

August 28, 2023 22:42

sreya commented

Aug 28, 2023

View reviewed changes

provisioner/terraform/testdata/display-apps/display-apps.tf OutdatedShow resolvedHide resolved

sreya requested a review frommtojek

August 28, 2023 22:43

feat: add display_apps field to agent

a71e3c8

sreya force-pushed thejon/defaultapps branch 3 times, most recently fromfda5adc toa71e3c8Compare

August 28, 2023 23:41

fix frontend

1ba035c

mtojek reviewed

Aug 29, 2023

View reviewed changes

Copy link

Member

mtojek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The first review round is done. Good job!

provisionersdk/agent.go OutdatedShow resolvedHide resolved

...ioner/terraform/testdata/resource-metadata-duplicate/resource-metadata-duplicate.tfplan.json

		@@ -1,6 +1,6 @@
		{

Copy link

Member

mtojekAug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There are many files changes intestdata, can we limit changes to only relevant ones?

Copy link

CollaboratorAuthor

sreyaAug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I just ran thegenerate.sh script 🤷 . I could remove all the unrelated changes but given I'm just running our dogfood image I don't think it's a big deal to go ahead and update them now. Especially if it'll keep happening to others. No strong opinion here though.

coderd/provisionerdserver/provisionerdserver.goShow resolvedHide resolved

provisioner/terraform/testdata/display-apps/display-apps.tf OutdatedShow resolvedHide resolved

coderd/database/modelmethods.go OutdatedShow resolvedHide resolved

coderd/provisionerdserver/provisionerdserver_test.goShow resolvedHide resolved

codersdk/workspaceagents.go

		type DisplayApp string

		const (
		DisplayAppVSCodeDesktop DisplayApp = "vscode"

Copy link

Member

mtojekAug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

QQ: do we need to take any action on the API level based these enum values? Maybecodersdk/API can be just a "dumb proxy", I mean "any" app.

Copy link

CollaboratorAuthor

sreyaAug 29, 2023•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

No not really it's mainly for discoverability and it also helps with the generated typescript types but I also don't mind removing it, wdyt?

Copy link

Member

mtojekAug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

My idea is to reduce the number of places to update when you add another application, so if we don't intend to use let's just drop it. Unless we will use it in TypeScript/site, then we can leave it 👍