Skip to content

feat(site): support match option for auto create workspace flow #13836

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jul 9, 2024

Conversation

BrunoQuaresma
Copy link
Collaborator

This feature will support having a match property in coder.yaml for the Coder Chrome plugin.

Related to https://github.com/coder/chrome-coder/issues/1

@BrunoQuaresma BrunoQuaresma requested a review from Parkreiner July 8, 2024 20:28
@BrunoQuaresma BrunoQuaresma self-assigned this Jul 8, 2024
Copy link
Member

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Looks good! Mainly had a question for my own understanding of the E2E tests

const richParameters: RichParameter[] = [
{ ...emptyParameter, name: "repo", type: "string" },
];
const template = await createTemplate(
Copy link
Member

Choose a reason for hiding this comment

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

I haven't worked with the E2E stuff much yet, so just to make sure I'm understanding: these tests aren't isolated, right? That's how the second test is able to have a workspace already exist, even though it didn't explicitly make two of them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are not isolated, but I ensure that a previous workspace with the right match exists in line 39.

});

onCreateWorkspace(newWorkspace);
} catch (err) {
searchParams.delete("mode");
setSearchParams(searchParams);
setMode("form");
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it might still be good to update the search params on top of the state change, but good catch with this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've thought about this and I don't think it makes much of a difference in the user experience, based on what I can see. Since no other part of the component uses it, I just removed it and decided to set the mode only in the state.

@BrunoQuaresma BrunoQuaresma enabled auto-merge (squash) July 9, 2024 16:13
@BrunoQuaresma BrunoQuaresma merged commit 6bf7e5a into main Jul 9, 2024
29 checks passed
@BrunoQuaresma BrunoQuaresma deleted the bq/support-match branch July 9, 2024 16:14
@github-actions github-actions bot locked and limited conversation to collaborators Jul 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants