[v9.1 backport] Updates `tsh ls` for node/app/db/kube to accept new filter flags #11016

kimlisa · 2022-03-10T00:00:37Z

Backport #10980 to branch/v9

Before Merge

merge after v9 release? (needs to be part of v9.1 release)

espadolini

Is there another PR for the docs for this (and the corresponding tctl changes)?

kimlisa · 2022-03-10T16:54:06Z

@espadolini this is the docs PR: #11012

api/client/client.go

zmb3 · 2022-03-10T17:11:42Z

lib/client/api.go

-	return proxyClient.GetDatabaseServers(ctx, tc.Namespace)
+
+	filter := customFilter
+	if customFilter == nil {


Suggested change

if customFilter == nil {

if filter == nil {

The end result is equivalent, but you're trying to set filter if not already set, so I think this check is easier to reason about.

zmb3 · 2022-03-10T17:13:45Z

lib/client/api_test.go

+func TestParseSearchKeywords(t *testing.T) {
+	t.Parallel()
+
+	expected := [][]string{


Can you make expected a field of the testCases anonymous struct?

When writing a new test it's nice to see the input and expected output together, rather than updating them in two separate places.

sorry zac, i wasn't sure what you meant by updating them in two places? the expected values should not change? i put it out b/c they are the same expects for each tests that test with similar inputs but with different delimiters.

This is what it looks like if something does not match (which looks easy to understand, to me at least):

maybe my test is confusing? should i create individual tests for each delimiter?

i separated the test to make it easier to understand: 1adccd8

zmb3 · 2022-03-10T17:15:02Z

lib/client/api_test.go

+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()


I would remove the parallel here, and then there's no need for the tc := tc line.

These tests will be super fast (they're pure computation, no network calls or anything that can block), so no need to further parallelize them.

zmb3 · 2022-03-10T17:15:20Z

lib/client/client.go

+
+	resources, err := client.GetResourcesWithFilters(ctx, site, req)
+	if err != nil {
+		// ListResources for nodes not availalbe, provide fallback.


Suggested change

// ListResources for nodes not availalbe, provide fallback.

// ListResources for nodes not available, provide fallback.

zmb3 · 2022-03-10T17:16:00Z

lib/client/client.go

@@ -663,13 +717,32 @@ func (proxy *ProxyClient) DeleteAppSession(ctx context.Context, sessionID string
 	return nil
 }

-// GetDatabaseServers returns all registered database proxy servers.
-func (proxy *ProxyClient) GetDatabaseServers(ctx context.Context, namespace string) ([]types.DatabaseServer, error) {
+// FindDatabaseServersByFilters returns all registered database proxy servers.


Suggested change

// FindDatabaseServersByFilters returns all registered database proxy servers.

// FindDatabaseServersByFilters returns registered database proxy servers that match the provided filter.

Technically it doesn't return all database servers.

lib/client/api.go

) * Also adds a search keyword parser that takes in different delimiters (comma is used for tsh, space is used for web UI) part of RFD 55

Addressing minor code reviews received on a backport (mostly grammer fixes and updating a test)

Addressing minor code reviews received on a backport

kimlisa changed the title ~~[v9.1 backport] Updates tsh ls for node/app/db/kube to accept new filter flags (#10…~~ [v9.1 backport] Updates tsh ls for node/app/db/kube to accept new filter flags Mar 10, 2022

github-actions bot added kubernetes-access tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Mar 10, 2022

github-actions bot requested review from espadolini, rosstimothy and zmb3 March 10, 2022 00:01

espadolini approved these changes Mar 10, 2022

View reviewed changes

zmb3 reviewed Mar 10, 2022

View reviewed changes

rosstimothy approved these changes Mar 11, 2022

View reviewed changes

lib/client/api.go Outdated Show resolved Hide resolved

lib/client/api.go Outdated Show resolved Hide resolved

lib/client/api.go Outdated Show resolved Hide resolved

kimlisa mentioned this pull request Mar 14, 2022

[v9.1 backport] Update 'tctl apps/db/nodes ls' to accept filter flags (#11003) #11076

Merged

2 tasks

kimlisa added 2 commits March 14, 2022 11:41

Updates tsh ls for node/app/db/kube to accept new filter flags (#10980

12b451e

) * Also adds a search keyword parser that takes in different delimiters (comma is used for tsh, space is used for web UI) part of RFD 55

Address CR part 1

d63e507

kimlisa force-pushed the lisa/v9/backport-tsh-ls-update branch from bf8095d to d63e507 Compare March 14, 2022 19:32

Make test easier to understand

1adccd8

kimlisa requested a review from zmb3 March 16, 2022 19:29

russjones approved these changes Mar 17, 2022

View reviewed changes

zmb3 approved these changes Mar 18, 2022

View reviewed changes

Merge branch 'branch/v9' into lisa/v9/backport-tsh-ls-update

726e937

kimlisa enabled auto-merge (squash) March 18, 2022 15:43

Merge branch 'branch/v9' into lisa/v9/backport-tsh-ls-update

aeeffee

kimlisa merged commit 7499530 into branch/v9 Mar 18, 2022

kimlisa deleted the lisa/v9/backport-tsh-ls-update branch March 18, 2022 16:28

kimlisa mentioned this pull request Mar 22, 2022

Add label filtering to tsh db ls #11194

Closed

webvictim mentioned this pull request Apr 19, 2022

opened in error #12068

Closed

kimlisa mentioned this pull request Apr 20, 2022

[forward port] Grammar fixes and updating a test #12115

Merged

kimlisa added a commit that referenced this pull request Apr 20, 2022

Forward porting (#11016)

46800f4

Addressing minor code reviews received on a backport (mostly grammer fixes and updating a test)

kimlisa added a commit that referenced this pull request Apr 20, 2022

[forward port] Grammar fixes and updating a test (#11016) (#12115)

ad1f826

Addressing minor code reviews received on a backport

webvictim mentioned this pull request Jun 8, 2022

Opened in error #13285

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v9.1 backport] Updates `tsh ls` for node/app/db/kube to accept new filter flags #11016

[v9.1 backport] Updates `tsh ls` for node/app/db/kube to accept new filter flags #11016

kimlisa commented Mar 10, 2022 •

edited

Loading

espadolini left a comment

kimlisa commented Mar 10, 2022

zmb3 Mar 10, 2022

zmb3 Mar 10, 2022

kimlisa Mar 14, 2022

kimlisa Mar 14, 2022

zmb3 Mar 10, 2022

zmb3 Mar 10, 2022

zmb3 Mar 10, 2022

	// ListResources for nodes not availalbe, provide fallback.
	// ListResources for nodes not available, provide fallback.

	// FindDatabaseServersByFilters returns all registered database proxy servers.
	// FindDatabaseServersByFilters returns registered database proxy servers that match the provided filter.

[v9.1 backport] Updates tsh ls for node/app/db/kube to accept new filter flags #11016

[v9.1 backport] Updates tsh ls for node/app/db/kube to accept new filter flags #11016

Conversation

kimlisa commented Mar 10, 2022 • edited Loading

Before Merge

espadolini left a comment

Choose a reason for hiding this comment

kimlisa commented Mar 10, 2022

zmb3 Mar 10, 2022

Choose a reason for hiding this comment

zmb3 Mar 10, 2022

Choose a reason for hiding this comment

kimlisa Mar 14, 2022

Choose a reason for hiding this comment

kimlisa Mar 14, 2022

Choose a reason for hiding this comment

zmb3 Mar 10, 2022

Choose a reason for hiding this comment

zmb3 Mar 10, 2022

Choose a reason for hiding this comment

zmb3 Mar 10, 2022

Choose a reason for hiding this comment

[v9.1 backport] Updates `tsh ls` for node/app/db/kube to accept new filter flags #11016

[v9.1 backport] Updates `tsh ls` for node/app/db/kube to accept new filter flags #11016

kimlisa commented Mar 10, 2022 •

edited

Loading