Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add open_in option to coder_app #321

Merged
merged 6 commits into from
Dec 19, 2024
Merged

add open_in option to coder_app #321

merged 6 commits into from
Dec 19, 2024

Conversation

defelmnq
Copy link
Contributor

@defelmnq defelmnq commented Dec 4, 2024

Working on #297

@defelmnq defelmnq self-assigned this Dec 4, 2024
mtojek
mtojek reviewed Dec 4, 2024
View reviewed changes
Copy link
Member

@mtojek 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, AFAIR make gen command should help here?

provider/app_test.go Show resolved Hide 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
mafredri approved these changes Dec 4, 2024
View reviewed changes
Copy link
Member

@mafredri 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 Outdated Show resolved Hide resolved
provider/app.go Show resolved Hide resolved
johnstcn
johnstcn reviewed Dec 16, 2024
View reviewed changes
Copy link
Member

@johnstcn 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

@matifali
Copy link
Member

matifali commented Dec 18, 2024 •
edited
Loading

Will it work the same for external=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 a coder_app that opens a URI that launches another app.

@defelmnq
Copy link
Contributor Author

Will it work the same for external=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 a coder_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.

@defelmnq defelmnq mentioned this pull request Dec 18, 2024
Open
@defelmnq defelmnq merged commit d933a71 into main Dec 19, 2024
8 checks passed
@defelmnq defelmnq deleted the open_in-parameter branch December 19, 2024 15:35
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mafredri mafredri mafredri approved these changes

@mtojek mtojek mtojek approved these changes

@johnstcn johnstcn johnstcn approved these changes

@DanielleMaywood DanielleMaywood Awaiting requested review from DanielleMaywood

Assignees

@defelmnq defelmnq

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@defelmnq @matifali @mafredri @johnstcn @mtojek