Web: define discover resource guide ids and add discover user preference field by kimlisa · Pull Request #51672 · gravitational/teleport

kimlisa · 2025-01-30T19:20:24Z

Part of improving the enroll resources page in the web UI, which is to allow user to pin favored resource guides. This PR just defines the user preference fields, the UI work to allow user to pin will be in another PR.

kimlisa · 2025-01-30T19:27:34Z

Alternative is to define these as proto enums. This takes more lines of code, but advantage is probably that it's harder to modify it? And stricter typing (like you can't just store any strings, it has to be a type of enum).

I opted to go for the simpler approach and see what kind of review I get.

i think enums client side like you have it is fine.

Yeah looks good since there isn't extra processing done by the backend on these id values and are meant just for the UI. The downside are that we must make sure that the id names (mostly words, spells etc) are correct the first time we merge to master as changing them later risks backend data and id values going out of sync. That wont be case if we define this in backend first and then copy 1:1 to frontend.

avatus · 2025-01-30T22:24:22Z

i think enums client side like you have it is fine.

avatus · 2025-01-30T22:30:41Z

+  DatabaseAwsSqlServerAd = 'database-aws-sql-server-ad',
+  DatabaseAwsPostgresRedshift = 'database-aws-postgres-redshift',
+  DatabaseAwsRdsPostgresSql = 'database-aws-rds-postgres-sql',
+  DatabaseAwsRdsProxyPostgres = 'database-aws-rds-proxy-postgres',


i dont really know what the protocol is here if we should call it postgres or postgresql or whatever. i think weve been pretty inconsistent in the rest of the code base. feel free to skip if you want (but when using the full name, lets keep it one word postgresql)

just realized i've been misspelling it (the extra s in postgressql), i decided to replace all postgressql with its shortened version postgres

avatus · 2025-01-30T22:31:22Z

+  DatabaseAzureSqlServerAd = 'database-azure-sql-server-ad',
+
+  DatabaseGcpMysqlCloudSql = 'database-gcp-mysql-cloud-sql',
+  DatabaseGcpPostgresCloudSql = 'database-gcp-postgres-cloud-sql',


Suggested change

DatabaseGcpPostgresCloudSql = 'database-gcp-postgres-cloud-sql',

DatabaseGcpPostgresCloudSql = 'database-gcp-postgres-cloud-sql',

PostgresqlCloud?

keeping it as is, it's actually how its named here: https://goteleport.com/docs/enroll-resources/database-access/enroll-google-cloud-databases/postgres-cloudsql/

flyinghermit

Looks good

flyinghermit · 2025-01-31T18:53:35Z

+message DiscoverGuidePreferences {
+  // pinned_guides is a list of ids of pinned guides.
+  repeated string pinned_guides = 1;
+}
+
+// DiscoverResourcePreferences holds preferences related to discovering resource.
+message DiscoverResourcePreferences {
+  DiscoverGuidePreferences discover_guide_preferences = 1;
+}


Curious, do we have plans to add more configuration to the guide preference?

Otherwise putting pinned guide directly in the discover resource preference seem like a better type to me.

message DiscoverResourcePreferences { // pinned_guides is a list of ids of pinned guides. repeated string pinned_guides = 1; }

Glad you pointed it out b/c i wanted to double check.

Curious, do we have plans to add more configuration to the guide preference?

probably not, i was partly following the existing pattern for pinning here and mostly because i needed a way to tell from init value vs user intentionally setting it empty

i use the nil value to render default pinned items

pinned results will only be saved if a user ever updates any pinned items

a user can also unpin everything leading to a empty list

you could also say DiscoverResourcePreferences can also be nil? but we could add more settings later that may not affect the pinned results

let me know what you think

i needed a way to tell from init value vs user intentionally setting it empty

Why do we want to check this?

Unless we know we need it 100% later on, I say lets settle for simple one like this. Any more settings you want to add can be added directly or wrapped in another message when necessary.

Why do we want to check this?

we want to display default pinned guides like this (default == nil preference):

if a user wanted to just unpin all of them (empty list), i needed a way to tell if they did that, otherwise the default pins will always render which is unexpected behavior.

or, are you saying just always render default if empty list?

actually, i think i can do something like this

DiscoverPreference { repeated string pinned_guides `...yada yada, omitempty` }

if i remember correctly, the omitempty allows for js to distinguish between value undefined and a empty length array. i'll test it

Yeah my thinking was to show out set of predefined defaults if the pinned_guides choice is empty. Any issue with that?

flyinghermit · 2025-01-31T18:56:38Z

+option go_package = "github.com/gravitational/teleport/api/gen/proto/go/userpreferences/v1;userpreferencesv1";
+
+// DiscoverGuidePreferences holds preferences related to discover resource guides.
+message DiscoverGuidePreferences {


nit: not sure if it will be confusing in practice but just looking at it, I think we can leave out Preferences suffix as this will ultimately be used as userpreferencesv1.DiscoverGuide. userpreferencesv1.DiscoverGuidePreferences reads redundant.

Suggested change

message DiscoverGuidePreferences {

message DiscoverGuide {

(same comment below)

keeping it as is, since it's the established pattern 😅

flyinghermit · 2025-01-31T18:58:40Z

+	PinnedGuides []string `json:"pinnedGuides"`
+}
+
+type DiscoverResourcePreferencesResponse struct {


flyinghermit · 2025-01-31T19:00:05Z

+	ClusterPreferences          ClusterUserPreferencesResponse      `json:"clusterPreferences,omitempty"`
+	DiscoverResourcePreferences DiscoverResourcePreferencesResponse `json:"discoverResourcePreferences"`
+	AccessGraph                 AccessGraphPreferencesResponse      `json:"accessGraph,omitempty"`


should DiscoverResourcePreferences also define omitempty or we are ok sending empty fields?

recording slack discussion: DiscoverResourcePreferences should always be initialized since i am using the undefined value as a way to determine if response came from an older proxy

flyinghermit · 2025-01-31T19:00:30Z

+// accessGraphPreferencesResponse creates a JSON response for the access graph preferences.
+func discoverResourcePreferenceResponse(resp *userpreferencesv1.DiscoverResourcePreferences) DiscoverResourcePreferencesResponse {


nit: update comment

flyinghermit · 2025-01-31T19:05:38Z

+
+// accessGraphPreferencesResponse creates a JSON response for the access graph preferences.
+func discoverResourcePreferenceResponse(resp *userpreferencesv1.DiscoverResourcePreferences) DiscoverResourcePreferencesResponse {
+	if resp == nil || resp.DiscoverGuidePreferences == nil {


nit: use GetX where possible as it adds safety from nil pointer

Suggested change

if resp == nil || resp.DiscoverGuidePreferences == nil {

if resp == nil || resp.GetDiscoverGuidePreferences == nil {

flyinghermit · 2025-01-31T19:08:07Z

+        r.id === DiscoverGuideId.ApplicationSamlGeneric ||
+        r.id === DiscoverGuideId.ApplicationSamlGrafana
+      ) {
+        return 'Teleport as IDP';


where does this name show up?

it shows up in the select a resource to enroll tile :

flyinghermit · 2025-01-31T19:10:17Z

   * An empty array or undefined means that the resource supports all auth types.
   */
  supportedAuthTypes?: AuthType[];
+  id: DiscoverGuideId;


curious, is is deliberate that this new field added as last item but another new field pinned?: boolean; added as first item?

not deliberate 😅 , i updated to put both fields at top since it's more fitting

flyinghermit · 2025-01-31T19:15:14Z

+ *
+ * Existing enum values must not be modified.
+ */
+export enum DiscoverGuideId {


nit: In favor of shorter names (since this will be used to pin the resource internally and not exposed to users right?

Application can be App

ApplicationSaml can be Saml

Database can be `Db

Applied to both key and values.

Feel free to ignore if you think we gain by spelling out loud.

keeping it as is

public-teleport-github-review-bot · 2025-02-04T23:33:01Z

@kimlisa See the table below for backport results.

Branch	Result
branch/v17	Failed

…nce field (#51672) * Add discover resource preferences to user preferences * Define hard coded guide id consts * Set guide ids for resources * Add test * Address CRs * Define a new type just for SelectResource.tsx which requires id field

…nce field (gravitational#51672) * Add discover resource preferences to user preferences * Define hard coded guide id consts * Set guide ids for resources * Add test * Address CRs * Define a new type just for SelectResource.tsx which requires id field

…52176) * Web: make SelectResource.tsx component leaner (#51698) * Web: Move resource specs into resource directory * Web: move code out into separate files * Web: define discover resource guide ids and add discover user preference field (#51672) * Add discover resource preferences to user preferences * Define hard coded guide id consts * Set guide ids for resources * Add test * Address CRs * Define a new type just for SelectResource.tsx which requires id field * Wrap discover guide preference "pinned" field inside a proto message (#52017) This allows us to use "nil" value to mean no discover guide preferences set (which is used to set default values in the web UI). * Web: Add filters and pinning support for Enroll Resources page (#51893) * Minor changes - Stop propagation and default behavior when clicking on pin button - Add small or large icon option for discover icons * Add pinning guide support * Fix lint

github-actions Bot added size/md ui labels Jan 30, 2025

github-actions Bot requested review from avatus and flyinghermit January 30, 2025 19:20

kimlisa commented Jan 30, 2025

View reviewed changes

kimlisa added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Jan 30, 2025

avatus approved these changes Jan 30, 2025

View reviewed changes

flyinghermit approved these changes Jan 31, 2025

View reviewed changes

kimlisa force-pushed the lisa/discover-guide-ids branch from 7ae8dae to 26da534 Compare February 4, 2025 19:41

kimlisa added 6 commits February 4, 2025 13:51

Add discover resource preferences to user preferences

6fe10a5

Define hard coded guide id consts

166725a

Set guide ids for resources

efa86af

Add test

1d1469e

Address CRs

55a7fe6

Define a new type just for SelectResource.tsx which requires id field

6b1c121

kimlisa force-pushed the lisa/discover-guide-ids branch from c4435e2 to 6b1c121 Compare February 4, 2025 21:52

kimlisa added this pull request to the merge queue Feb 4, 2025

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Feb 4, 2025

kimlisa added this pull request to the merge queue Feb 4, 2025

Merged via the queue into master with commit 4312c5f Feb 4, 2025

kimlisa deleted the lisa/discover-guide-ids branch February 4, 2025 23:31

kimlisa mentioned this pull request Feb 14, 2025

[v17] Web: Add filters and pinning support for Enroll Resources page #52176

Merged

	DatabaseGcpPostgresCloudSql = 'database-gcp-postgres-cloud-sql',
	DatabaseGcpPostgresCloudSql = 'database-gcp-postgres-cloud-sql',

		// accessGraphPreferencesResponse creates a JSON response for the access graph preferences.
		func discoverResourcePreferenceResponse(resp *userpreferencesv1.DiscoverResourcePreferences) DiscoverResourcePreferencesResponse {

	if resp == nil \|\| resp.DiscoverGuidePreferences == nil {
	if resp == nil \|\| resp.GetDiscoverGuidePreferences == nil {

Conversation

kimlisa commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimlisa Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimlisa Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flyinghermit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimlisa Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Feb 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

kimlisa commented Jan 30, 2025 •

edited

Loading

kimlisa Feb 4, 2025 •

edited

Loading

kimlisa Feb 4, 2025 •

edited

Loading

kimlisa Jan 31, 2025 •

edited

Loading