feat: add OpenIn option to coder_app #15743

defelmnq · 2024-12-04T13:02:30Z

This PR is the coder/coder part of the open_in parameter issue aiming to add a new optional parameter to choose how to open modules.

This PR is heavily linked to this PR.

ℹ️ For now, some integrations tests can not be pushed as it requires a release on the terraform-provider repo.

mtojek

Nice work! LGTM 👍 ... but please wait for double-check from @johnstcn

You may want to mention the next steps like:

adjust Coder UI
update the dependency on terraform-provider-coder
etc.

coderd/database/migrations/000279_workspace_app_add_open_in.up.sql

johnstcn

LGTM, only suggestion I have is to define open_in as an enum instead, similar to how some other fields of the table are defined.

coderd/database/dump.sql

coderd/database/migrations/000279_workspace_app_add_open_in.up.sql

defelmnq · 2024-12-23T22:34:56Z

@mtojek @johnstcn I modified a lot of the code since your last review to add :

Enum on the database
Enum in proto files in the protosdk

Can I ask you for another stamp ? 🙏

CI is ok but just waiting for storybook review (which are false positive, nothing changed.)

coderd/provisionerdserver/provisionerdserver.go

mtojek · 2024-12-24T09:25:18Z

I reviewed Storybook images and they seem to be correct 👍

johnstcn · 2024-12-24T09:22:36Z

provisionersdk/proto/provisioner.proto

When adding a new field to this protobuf, you will need to increment provisionerd.proto.CurrentMinor.

TIL - is it documented somewhere? I believe there is no linting rule.

I'm second-guessing myself now -- we definitely need to increment minor version when adding a new RPC method.

Yep, no need to increment here. Sorry for the confusion!

We do need to increment the minor version:

https://www.notion.so/coderhq/API-Changes-Back-Compatibility-36e779e691e140d4a3dcb78700c6d0bf?pvs=4#21fc01f1d65847d1a2cff6a2558def76

This is a new feature, not a bug fix, so it gets a minor version increment.

Apologies for the confusion! I opened a separate PR to add some guidelines so we don't get confused about this in future: #16009

codersdk/workspaceapps.go

johnstcn · 2024-12-24T09:26:15Z

site/src/api/typesGenerated.ts

@@ -2559,6 +2559,7 @@ export interface WorkspaceApp {
    readonly healthcheck: Healthcheck;
    readonly health: WorkspaceAppHealth;
    readonly hidden: boolean;
+    readonly open_in: string;


Defining this as a typed const in the Go code will mean this becomes a proper type in the generated TypeScript code.

johnstcn · 2024-12-24T09:32:44Z

agent/proto/agent.proto

I think we'll also need to create a new ConnectRPC24 method in codersdk/agentsdk to handle this new field, which defines an API version of major = 2 and minor = 4 adding this new field. Correct if I'm wrong @spikecurtis ?

Looking through #15508 again, it looks like I'm making much ado about nothing. We only need to bump the version when we modify the method interface, but adding a struct field to a proto shouldn't cause any issues. Please disregard the above comment.

bump api versions

johnstcn

requesting changes to clarify changes to api versions

johnstcn

Clarified api version changes. I think we still should define the externally-facing codersdk field as a typed const, but I think that's the last piece. Re-approving to un-block.

johnstcn · 2024-12-24T09:51:53Z

agent/proto/agent.proto

Looking through #15508 again, it looks like I'm making much ado about nothing. We only need to bump the version when we modify the method interface, but adding a struct field to a proto shouldn't cause any issues. Please disregard the above comment.

johnstcn · 2024-12-24T09:53:00Z

provisionersdk/proto/provisioner.proto

Yep, no need to increment here. Sorry for the confusion!

spikecurtis · 2025-01-02T08:16:16Z

provisionersdk/proto/provisioner.proto

We do need to increment the minor version:

https://www.notion.so/coderhq/API-Changes-Back-Compatibility-36e779e691e140d4a3dcb78700c6d0bf?pvs=4#21fc01f1d65847d1a2cff6a2558def76

This is a new feature, not a bug fix, so it gets a minor version increment.

matifali · 2025-01-02T08:47:59Z

go.mod

@@ -100,7 +100,7 @@ require (
 	github.com/coder/quartz v0.1.2
 	github.com/coder/retry v1.5.1
 	github.com/coder/serpent v0.10.0
-	github.com/coder/terraform-provider-coder v1.0.2
+	github.com/coder/terraform-provider-coder v1.0.4


Shouldn't this go to v2.1.0?

@defelmnq I am certainly not familiar with the code base as you but why we are still on version 1.0.4 of provider here?

defelmnq

All comments should be fixed and the PR ready to merge - I tested in dogfood and everything's working.

cc @spikecurtis for the version bump.

coderd/provisionerdserver/provisionerdserver.go

codersdk/workspaceapps.go

spikecurtis

One comment inline, but I don't need to review again.

provisionerd/proto/version.go

work on OpenIn implementation

5483426

defelmnq self-assigned this Dec 4, 2024

defelmnq and others added 2 commits December 4, 2024 14:25

improve open_in parameter handling

1752f40

Update provisioner.proto

0e69ff4

github-actions bot added the stale This issue is like stale bread. label Dec 12, 2024

github-actions bot closed this Dec 15, 2024

johnstcn reopened this Dec 16, 2024

github-actions bot removed the stale This issue is like stale bread. label Dec 17, 2024

defelmnq added 2 commits December 17, 2024 01:45

add open_in logic

bd9d40b

add open_in logic

8596e92

defelmnq changed the title ~~Add OpenIn option to coder_app~~ feat: add OpenIn option to coder_app Dec 17, 2024

defelmnq added 2 commits December 17, 2024 23:07

Merge remote-tracking branch 'origin/main' into coder_app-open_in

9a99df5

up generated doc

451bf3b

defelmnq force-pushed the coder_app-open_in branch from e5496a7 to 451bf3b Compare December 18, 2024 00:07

defelmnq and others added 4 commits December 18, 2024 00:17

work on ts tests

6b8858e

work on ts tests

90fd0e3

work on tests for tf

5c207ec

reset failing tests

803fe24

defelmnq marked this pull request as ready for review December 18, 2024 14:41

defelmnq requested review from spikecurtis and johnstcn as code owners December 18, 2024 14:41

defelmnq requested review from mtojek and removed request for spikecurtis December 18, 2024 14:41

mtojek approved these changes Dec 19, 2024

View reviewed changes

coderd/database/migrations/000279_workspace_app_add_open_in.up.sql Outdated Show resolved Hide resolved

johnstcn previously approved these changes Dec 19, 2024

View reviewed changes

coderd/database/dump.sql Outdated Show resolved Hide resolved

coderd/database/migrations/000279_workspace_app_add_open_in.up.sql Outdated Show resolved Hide resolved

defelmnq and others added 5 commits December 20, 2024 13:28

Merge remote-tracking branch 'origin/main' into coder_app-open_in

82d4f8f

bump go.mod

f97a385

merge

bfdaefd

change database type for enum

c6d18a6

change database type for enum

b81b84d

defelmnq added 3 commits December 23, 2024 21:17

work on tests in dbmem

079e474

work on tests

6e368bf

work on tests

1dec106

defelmnq requested review from johnstcn and mtojek December 23, 2024 22:34

mtojek reviewed Dec 24, 2024

View reviewed changes

coderd/provisionerdserver/provisionerdserver.go Outdated Show resolved Hide resolved

coderd/provisionerdserver/provisionerdserver.go Outdated Show resolved Hide resolved

johnstcn reviewed Dec 24, 2024

View reviewed changes

johnstcn requested changes Dec 24, 2024

View reviewed changes

johnstcn approved these changes Dec 24, 2024

View reviewed changes

github-actions bot added the stale This issue is like stale bread. label Jan 1, 2025

spikecurtis requested changes Jan 2, 2025

View reviewed changes

matifali reviewed Jan 2, 2025

View reviewed changes

github-actions bot removed the stale This issue is like stale bread. label Jan 3, 2025

defelmnq requested a review from spikecurtis January 3, 2025 02:39

defelmnq added 7 commits January 3, 2025 02:54

increase minor version of provisioner api

54f3a70

Merge remote-tracking branch 'origin/main' into coder_app-open_in

5730ea0

improve constant and code logic

8f76203

increase migration idx

4ca1aa0

Merge remote-tracking branch 'origin/main' into coder_app-open_in

c64973d

increase migration idx

4698162

make gen

2122b3b

defelmnq commented Jan 3, 2025

View reviewed changes

coderd/provisionerdserver/provisionerdserver.go Outdated Show resolved Hide resolved

coderd/provisionerdserver/provisionerdserver.go Outdated Show resolved Hide resolved

codersdk/workspaceapps.go Show resolved Hide resolved

spikecurtis approved these changes Jan 3, 2025

View reviewed changes

provisionerd/proto/version.go Show resolved Hide resolved

add version history in the provisioner version

bb801f0

defelmnq merged commit 08463c2 into main Jan 3, 2025
33 of 34 checks passed

defelmnq deleted the coder_app-open_in branch January 3, 2025 10:27

github-actions bot locked and limited conversation to collaborators Jan 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add OpenIn option to coder_app #15743

feat: add OpenIn option to coder_app #15743

defelmnq commented Dec 4, 2024 •

edited

Loading

mtojek left a comment

johnstcn left a comment

defelmnq commented Dec 23, 2024 •

edited

Loading

mtojek commented Dec 24, 2024

johnstcn Dec 24, 2024

mtojek Dec 24, 2024

johnstcn Dec 24, 2024

johnstcn Dec 24, 2024

spikecurtis Jan 2, 2025

johnstcn Jan 2, 2025

johnstcn Dec 24, 2024

johnstcn Dec 24, 2024

johnstcn Dec 24, 2024

johnstcn left a comment

johnstcn left a comment

johnstcn Dec 24, 2024

johnstcn Dec 24, 2024

spikecurtis Jan 2, 2025

matifali Jan 2, 2025

matifali Jan 3, 2025

defelmnq left a comment

spikecurtis left a comment

feat: add OpenIn option to coder_app #15743

feat: add OpenIn option to coder_app #15743

Conversation

defelmnq commented Dec 4, 2024 • edited Loading

mtojek left a comment

Choose a reason for hiding this comment

johnstcn left a comment

Choose a reason for hiding this comment

defelmnq commented Dec 23, 2024 • edited Loading

mtojek commented Dec 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn left a comment

Choose a reason for hiding this comment

johnstcn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defelmnq left a comment

Choose a reason for hiding this comment

spikecurtis left a comment

Choose a reason for hiding this comment

defelmnq commented Dec 4, 2024 •

edited

Loading

defelmnq commented Dec 23, 2024 •

edited

Loading