coder/terraform-provider-coderPublic

NotificationsYou must be signed in to change notification settings
Fork22
Star47

add open_in option to coder_app#321

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

defelmnq merged 6 commits intomainfromopen_in-parameter

Dec 19, 2024

Merged

add open_in option to coder_app#321

defelmnq merged 6 commits intomainfromopen_in-parameter

Dec 19, 2024

Conversation

Copy link

Contributor

defelmnq commentedDec 4, 2024

Working on#297

add open_in option to coder_app

6ca152d

defelmnq self-assigned this

Dec 4, 2024

mtojek requested review fromDanielleMaywood andmafredri

December 4, 2024 13:59

mtojek reviewed

Dec 4, 2024

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.

Linter complains about docs, AFAIRmake gen command should help here?

provider/app_test.goShow resolvedHide resolved

provider/app_test.go

		}
		`,
		hidden: false,
		openIn: "window",

Copy link

Member

Choose a reason for hiding this comment

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

You may want to add this also to examples, see:https://github.com/coder/terraform-provider-coder/blob/main/examples/resources/coder_app/resource.tf

mafredri approved these changes

Dec 4, 2024

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

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

Just a few thoughts on docs, otherwise LGTM 👍🏻

provider/app.go OutdatedShow resolvedHide resolved

provider/app.goShow resolvedHide resolved

johnstcn reviewed

Dec 16, 2024

View reviewed changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

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

LGTM, would like to see an integration test added but after that I'm happy

johnstcn approved these changes

Dec 16, 2024

View reviewed changes

defelmnq force-pushed theopen_in-parameter branch fromed5cdab to6ca152dCompare

December 17, 2024 01:55

defelmnq added5 commits

December 17, 2024 03:11

work on tests

3d5fc0b

add missing example

aa7c5c7

rename test

002062a

lint

de3c119

generate docs

37ba63e

Copy link

Member

matifali commentedDec 18, 2024•
edited
Loading

Will it work the same forexternal=true?
I am concerned about VSCode-Desktop, Cursor, and JetBrains Gateway modules. We should make sure that we do not leave an empty window when a user clicks on acoder_app that opens a URI that launches another app.

Copy link

ContributorAuthor

defelmnq commentedDec 18, 2024

Will it work the same forexternal=true? I am concerned about VSCode-Desktop, Cursor, and JetBrains Gateway modules. We should make sure that we do not leave an empty window when a user clicks on acoder_app that opens a URI that launches another app.

Yes, so for now it will - for the apps not-browser based - just be ignored. Anyway, your point makes complete sense and it can be a follow-up item to have validation to either have either external or open_in parameters.