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

Teleterm: add support for access requesting kube namespaces #47347

Merged
merged 1 commit into from
Oct 28, 2024
Merged

Teleterm: add support for access requesting kube namespaces #47347

merged 1 commit into from
Oct 28, 2024

Conversation

kimlisa
Copy link
Contributor

@kimlisa kimlisa commented Oct 8, 2024 •
edited
Loading

part of #46742

requires:

if you want to test connect, i have a staging cluster that i can invite you to (dm me) then:

  1. OS: check out this branch
  2. build tsh, and then pnpm start-term and login to my cluster

screenshots and demo similar as: #47345

changelog: Add Connect support for selecting Kubernetes namespaces during access requests

@github-actions github-actions bot requested review from gzdunek and ryanclark October 8, 2024 16:44
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 8, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@kimlisa kimlisa requested review from ravicious and removed request for ryanclark October 8, 2024 16:45
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 8, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@kimlisa kimlisa force-pushed the lisa/kube-namespace-3 branch from 768b594 to 3140a5c Compare October 9, 2024 06:13
@kimlisa kimlisa force-pushed the lisa/teleterm-kube-namespace branch from 357f75a to 65cb297 Compare October 9, 2024 06:17
@pnrao1983 pnrao1983 added the c-svt Internal Customer Reference label Oct 16, 2024
@kimlisa kimlisa force-pushed the lisa/kube-namespace-3 branch from 3140a5c to a7f49c2 Compare October 17, 2024 20:15
@kimlisa kimlisa requested a review from gzdunek October 18, 2024 07:29
@kimlisa kimlisa force-pushed the lisa/teleterm-kube-namespace branch from 65cb297 to 2042699 Compare October 18, 2024 07:45
@ravicious ravicious mentioned this pull request Oct 18, 2024
Merged
2 tasks
@kimlisa kimlisa force-pushed the lisa/teleterm-kube-namespace branch from 2042699 to 6902cd8 Compare October 21, 2024 06:17
@kimlisa kimlisa requested a review from ravicious October 21, 2024 06:23
@ravicious ravicious mentioned this pull request Oct 21, 2024
Merged
@ravicious
Copy link
Member

I'll take a look tomorrow and I'll try to use the UI.

ravicious
ravicious reviewed Oct 22, 2024
View reviewed changes
web/packages/shared/components/AccessRequests/NewRequest/RequestCheckout/RequestCheckout.tsx Outdated Show resolved Hide resolved
web/packages/shared/components/AccessRequests/NewRequest/RequestCheckout/RequestCheckout.tsx Outdated
Copy link
Member

Choose a reason for hiding this comment

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

idk how to create a minimal repro for this, but sometimes after closing the select, the list of selected namespaces gets sorted. In the video it happens right at the end.

Is it because we sort it at some point or is it because the namespaces are kept in a set in Connect? FWIW, I think Set in JS keeps insertion order, so idk if it's plausible for this data structure to cause that.

sort-after-select-close.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that doesn't look like sorting, but just rearranging? maybe it's b/c of how we bulk toggle

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it might be due to that, but looking at the code I can't quite understand why it happens. A smaller repro would be this:

  1. Select the first three namespaces (say they're called foo, bar and baz) and close the select.
  2. Open the select again. From the list, deselect the second item. Deselect the first item and select it again. Close the list.
  3. Before the list gets closed, foo is the last element (since it was most recently added), but when the list is closed, it is now the first element.

Could you add a test for this and fix it?

Copy link
Contributor Author

@kimlisa kimlisa Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is what's happening under the hood:

we start with this

image

later, i deselect coffee2, deselect and reselect coffee 1 and select coffee4, final result is this:

image

The order we expect is coffee3, coffee1, coffee4, but b/c we aren't replacing the entire array, it like keeps the previous order and just setting new namespaces and deleting ones we dont need, so the order gets re-arranged to coffee1, coffee3, coffee4

i can fix this by just replacing the entire array (this toggling worked better with the previous data modeling, and web UI will also needs to change i think), but i would like to punt this to a separate PR and not block this one.

web/packages/teleterm/src/ui/DocumentAccessRequests/NewRequest/useNewRequest.ts Outdated Show resolved Hide resolved
@kimlisa kimlisa force-pushed the lisa/kube-namespace-3 branch from 7b9a051 to 638a391 Compare October 22, 2024 21:16
@kimlisa kimlisa force-pushed the lisa/teleterm-kube-namespace branch 3 times, most recently from 939680d to c7802cb Compare October 23, 2024 05:33
@kimlisa kimlisa requested review from ravicious and gzdunek October 23, 2024 05:43
gzdunek
gzdunek approved these changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

With some minor comments.

web/packages/teleterm/src/ui/services/workspacesService/accessRequestsService.ts Show resolved Hide resolved
web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts Outdated Show resolved Hide resolved
web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts Outdated Show resolved Hide resolved
web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts Outdated Show resolved Hide resolved
@kimlisa kimlisa force-pushed the lisa/kube-namespace-3 branch from 638a391 to 1a979b2 Compare October 24, 2024 01:52
@kimlisa kimlisa force-pushed the lisa/teleterm-kube-namespace branch from c7802cb to 9c96761 Compare October 24, 2024 03:50
@kimlisa kimlisa force-pushed the lisa/teleterm-kube-namespace branch from 9c96761 to 84cac38 Compare October 28, 2024 15:28
@kimlisa kimlisa force-pushed the lisa/kube-namespace-3 branch from 1a979b2 to d10cd23 Compare October 28, 2024 15:34
Base automatically changed from lisa/kube-namespace-3 to master October 28, 2024 16:06
@kimlisa kimlisa force-pushed the lisa/teleterm-kube-namespace branch from 84cac38 to c63dec9 Compare October 28, 2024 16:45
@kimlisa kimlisa added this pull request to the merge queue Oct 28, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 28, 2024
@kimlisa kimlisa added this pull request to the merge queue Oct 28, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 28, 2024
@kimlisa kimlisa added this pull request to the merge queue Oct 28, 2024
Merged via the queue into master with commit ac2f8dc Oct 28, 2024
41 checks passed
@kimlisa kimlisa deleted the lisa/teleterm-kube-namespace branch October 28, 2024 17:57
@public-teleport-github-review-bot
Copy link

@kimlisa See the table below for backport results.

Branch Result
branch/v16 Failed
branch/v17 Failed

kimlisa added a commit that referenced this pull request Oct 28, 2024
@kimlisa
Teleterm: add support for access requesting kube namespaces (#47347)
9f3b17a
@kimlisa kimlisa mentioned this pull request Oct 28, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Oct 28, 2024
@kimlisa
[v17] Web/Teleterm: add support for requesting for kube namespaces (#…
76d678d 
…48015)

* Web: add support for requesting for kube namespaces

* Teleterm: add support for access requesting kube namespaces (#47347)
kimlisa added a commit that referenced this pull request Nov 5, 2024
@kimlisa
Teleterm: add support for access requesting kube namespaces (#47347)
dbe2951
@kimlisa kimlisa mentioned this pull request Nov 5, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 5, 2024
@kimlisa
[v16] Add Connect and Web UI support for selecting Kubernetes namespa…
be89454 
…ces during access requests (#48413)

* Web: add support for requesting for kube namespaces (#47345)

* Teleterm: add support for access requesting kube namespaces (#47347)

* WebShared: Update how request checkout handles kube resource related errors (#48168)

* WebShared: Update how request checkout handles kube resource related errors

* Fix bug where after create/cancel, specifiable fields were retained

* Remove single toggler for kube resources

* Address CR

* Update snaps

* Backport fixes

- teleport version v16 and less uses react select version 3
  which required to add manual support for initially fetching
  namespaces on select dropdown
- hover tooltip design diffs
- field select design diffs
@ravicious ravicious mentioned this pull request Nov 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gzdunek gzdunek gzdunek approved these changes

@ravicious ravicious ravicious approved these changes

Assignees
No one assigned
Labels
backport/branch/v16 backport/branch/v17 c-svt Internal Customer Reference size/md ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kimlisa @ravicious @gzdunek @pnrao1983