- Notifications
You must be signed in to change notification settings - Fork921
feat(site): connect open_in parameter#16036
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
Uh oh!
There was an error while loading.Please reload this page.
codersdk/agentsdk/convert_test.go Outdated
| @@ -64,6 +65,7 @@ func TestManifest(t *testing.T) { | |||
| }, | |||
| Health: codersdk.WorkspaceAppHealthInitializing, | |||
| Hidden: true, | |||
| OpenIn: codersdk.WorkspaceAppOpenInTab, | |||
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.
We changed the test inputs and it passes with no futher changes? Are we not checking for this in the test?
Should we also add a test with an invalid value forOpenIn?
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 indeed will add some more tests 👀
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.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -518,6 +518,7 @@ func Apps(dbApps []database.WorkspaceApp, agent database.WorkspaceAgent, ownerNa | |||
| }, | |||
| Health: codersdk.WorkspaceAppHealth(dbApp.Health), | |||
| Hidden: dbApp.Hidden, | |||
| OpenIn: codersdk.WorkspaceAppOpenIn(dbApp.OpenIn), | |||
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.
This piece is the most important of the PR - basically extracting data from the db to make them be visible in the SDK and API calls.
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 your description might need updating now.
Also:
Also make sure it is available and forwarded up to the CreateLink component.
Is this present?
Nit:
This PR almost feels too small now; I'm happy to merge it as-is but it might make more sense to combine this change with the new frontend behavior so it's all logically grouped instead of spread out over several PRs.
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.
Approving to unblock; would still like to see a couple of additional tests but no other major blockers from my side.
289338f intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Second step toresolveopen_in issue
This PR improves the way the open_in parameter is forwarded across the code, changing the last
stringto const everywhere.Also make sure it is available and forwarded up to the
CreateLinkcomponent.