- Notifications
You must be signed in to change notification settings - Fork917
feat: add OpenIn option to coder_app#15743
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
e5496a7 to451bf3bCompareThere 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.
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 OutdatedShow resolvedHide resolved
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.
LGTM, only suggestion I have is to defineopen_in as an enum instead, similar to how some other fields of the table are defined.
Uh oh!
There was an error while loading.Please reload this page.
coderd/database/migrations/000279_workspace_app_add_open_in.up.sql OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
defelmnq commentedDec 23, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
I reviewed Storybook images and they seem to be correct 👍 |
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.
When adding a new field to this protobuf, you will need to incrementprovisionerd.proto.CurrentMinor.
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.
TIL - is it documented somewhere? I believe there is no linting rule.
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.
I'm second-guessing myself now -- we definitely need to increment minor version when adding a new RPC method.
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.
Yep, no need to increment here. Sorry for the confusion!
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.
Wedo need to increment the minor version:
This is a new feature, not a bug fix, so it gets a minor version increment.
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.
Apologies for the confusion! I opened a separate PR to add some guidelines so we don't get confused about this in future:#16009
Uh oh!
There was an error while loading.Please reload this page.
| @@ -2559,6 +2559,7 @@ export interface WorkspaceApp { | |||
| readonly healthcheck: Healthcheck; | |||
| readonly health: WorkspaceAppHealth; | |||
| readonly hidden: boolean; | |||
| readonly open_in: string; | |||
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.
Defining this as a typed const in the Go code will mean this becomes a proper type in the generated TypeScript code.
agent/proto/agent.proto Outdated
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.
I think we'll also need to create a newConnectRPC24 method incodersdk/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 ?
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.
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.
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.
requesting changes to clarify changes to api versions
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.
Clarified api version changes. I think we still should define the externally-facingcodersdk field as a typed const, but I think that's the last piece. Re-approving to un-block.
agent/proto/agent.proto Outdated
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.
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.
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.
Yep, no need to increment here. Sorry for the confusion!
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.
Wedo need to increment the minor version:
This is a new feature, not a bug fix, so it gets a minor version increment.
| @@ -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 | |||
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.
Shouldn't this go tov2.1.0?
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.
@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?
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 comments should be fixed and the PR ready to merge - I tested in dogfood and everything's working.
cc@spikecurtis for the version bump.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
One comment inline, but I don't need to review again.
Uh oh!
There was an error while loading.Please reload this page.
08463c2 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This PR is the coder/coder part ofthe open_in parameter issue aiming to add a new optional parameter to choose how to open modules.
This PR is heavily linkedto this PR.
ℹ️ For now, some integrations tests can not be pushed as it requires a release on the terraform-provider repo.