Add optional component props to Roles component #51322

avatus · 2025-01-22T00:07:14Z

This PR enables us to add an optional RoleDiff component to the props for our Roles feature so that we can create an "e wrapper" around the Roles feature for enterprise specifically. Then we can pass the Role Diff visualizer into the E route.

Adds support (for this stub) in https://github.com/gravitational/teleport.e/pull/5902

avatus · 2025-01-22T04:27:25Z

web/packages/teleport/src/Roles/RoleEditor/RoleEditorAdapter.tsx

+        {roleDiffProps ? (
+          <roleDiffProps.RoleDiffComponent />
+        ) : (
+          <Box maxWidth={promoImageWidth + 2 * 2} minWidth={300}>


the diff is a bit wild on this one due to a lot of similar child components, but i didnt change anything with the structure of the Coming Soon box, just moved it into the tertiary

Can you make this part a separate component that is passed in as RoleDiffComponent for the OSS version?

I can make it a separate component, sure. I didn't make it OSS only since e clusters will still need this if TAG isn't enabled

Ah fair enough. I think putting it in a separate component and importing it from e makes it a bit more obvious what's happening on both the e and OSS side

Makes sense. Ill update it

ryanclark · 2025-01-22T14:17:28Z

web/packages/teleport/src/Roles/Roles.tsx

+type RoleDiffProps =
+  | { RoleDiffComponent?: undefined; updateRoleDiff?: undefined }
+  | {
+      RoleDiffComponent: React.FC;


ComponentType

ryanclark · 2025-01-22T14:25:23Z

web/packages/teleport/src/Roles/RoleEditor/RoleEditorAdapter.tsx

+        {roleDiffProps ? (
+          <roleDiffProps.RoleDiffComponent />
+        ) : (
+          <Box maxWidth={promoImageWidth + 2 * 2} minWidth={300}>


Can you make this part a separate component that is passed in as RoleDiffComponent for the OSS version?

ryanclark · 2025-01-22T14:27:27Z

web/packages/teleport/src/Roles/Roles.tsx

+// RoleDiffProps are an optonal set of props to render the role diff visualizer.
+// This type union will ensure that either all fields are undefined or all are supplied
+type RoleDiffProps =
+  | { RoleDiffComponent?: undefined; updateRoleDiff?: undefined }


If roleDiffProps is optional, why would I pass rollDiffProps={{}} (which matches this type)?

yeah i think i might have overthought this one. updated to just make roleDiffProps optional and typed the RoleDiffProps normally

bl-nero · 2025-01-22T17:59:54Z

web/packages/teleport/src/Roles/PolicyPlaceholder.tsx

+      </Flex>
+    </Flex>
+  );
+}


This is just moved from the adapter component, right?

bl-nero · 2025-01-22T18:15:43Z

web/packages/teleport/src/Roles/Roles.tsx

@@ -48,15 +48,25 @@ import { RoleList } from './RoleList';
 import templates from './templates';
 import { State, useRoles } from './useRoles';

-export function RolesContainer() {
+// RoleDiffProps are an optonal set of props to render the role diff visualizer.


Suggested change

// RoleDiffProps are an optonal set of props to render the role diff visualizer.

// RoleDiffProps are an optional set of props to render the role diff visualizer.

bl-nero · 2025-01-22T18:16:49Z

web/packages/teleport/src/Roles/Roles.tsx

+// RoleDiffProps are an optonal set of props to render the role diff visualizer.
+type RoleDiffProps = {
+  RoleDiffComponent: ComponentType;
+  updateRoleDiff: (role: Role) => Promise<void>;


Is this a property that will be used in future, or something that you forgot to remove?

the updateRoleDiff method is what will be passed down to RoleEditorAdapter as a callback for when the standardModel updates (or yaml is previewed)

bl-nero · 2025-01-22T18:23:52Z

web/packages/teleport/src/features.tsx

@@ -320,6 +320,11 @@ export class FeatureRoles implements TeleportFeature {
  };

  showInDashboard = true;
+  // getRoute allows child class extending this
+  // parent class to refer to this parent's route.


I know that every feature class has this, but I'm still confused right now how the presence of this function makes the route field more accessible. What's wrong with super.route?

honestly idk. super.route wasn't functioning so i just followed prior work and moved on without diving in too deep. i did try that first tho

You can only access super's methods, not properties (unless the route was defined in the constructor, then this.route would work)

TIL. any class stuff ive ever done has ALWAYS had the stuff defined in the constructor so i guess thats why i never noticed

This PR enables us to add an optional RoleDiff component to the props for our Roles feature so that we can create an "e wrapper" around the Roles feature for enterprise specifically. Then we can pass the Role Diff visualizer into the E route.

public-teleport-github-review-bot · 2025-01-23T16:35:45Z

@avatus See the table below for backport results.

Branch	Result
branch/v17	Create PR

avatus added the no-changelog Indicates that a PR does not require a changelog entry label Jan 22, 2025

avatus requested review from ryanclark and bl-nero January 22, 2025 00:07

github-actions bot added size/md ui labels Jan 22, 2025

github-actions bot requested a review from gzdunek January 22, 2025 00:07

avatus removed the request for review from gzdunek January 22, 2025 00:08

avatus force-pushed the avatus/roles_e branch from 777f0fc to 12c4d7c Compare January 22, 2025 00:40

avatus commented Jan 22, 2025

View reviewed changes

ryanclark reviewed Jan 22, 2025

View reviewed changes

avatus force-pushed the avatus/roles_e branch from ccfeca0 to 0fafc92 Compare January 22, 2025 15:27

bl-nero approved these changes Jan 22, 2025

View reviewed changes

avatus requested a review from ryanclark January 22, 2025 18:37

avatus force-pushed the avatus/roles_e branch from f1a01d3 to cb23986 Compare January 22, 2025 22:18

avatus enabled auto-merge January 22, 2025 22:18

avatus added backport-required backport/branch/v17 labels Jan 22, 2025

ryanclark approved these changes Jan 23, 2025

View reviewed changes

avatus force-pushed the avatus/roles_e branch 2 times, most recently from 94d42c5 to 0cc8244 Compare January 23, 2025 14:48

avatus added this pull request to the merge queue Jan 23, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 23, 2025

avatus added this pull request to the merge queue Jan 23, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 23, 2025

avatus force-pushed the avatus/roles_e branch from 0cc8244 to c2ab0f2 Compare January 23, 2025 15:32

avatus added this pull request to the merge queue Jan 23, 2025

Merged via the queue into master with commit 928bb89 Jan 23, 2025
41 checks passed

avatus deleted the avatus/roles_e branch January 23, 2025 16:34

avatus mentioned this pull request Jan 23, 2025

[v17] Add optional component props to Roles component #51421

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional component props to Roles component #51322

Add optional component props to Roles component #51322

avatus commented Jan 22, 2025 •

edited

Loading

avatus Jan 22, 2025

ryanclark Jan 22, 2025

avatus Jan 22, 2025

ryanclark Jan 22, 2025

avatus Jan 22, 2025

ryanclark Jan 22, 2025

ryanclark Jan 22, 2025

ryanclark Jan 22, 2025

avatus Jan 22, 2025

bl-nero Jan 22, 2025

avatus Jan 22, 2025

bl-nero Jan 22, 2025

bl-nero Jan 22, 2025

avatus Jan 22, 2025

bl-nero Jan 22, 2025

avatus Jan 22, 2025

ryanclark Jan 23, 2025 •

edited

Loading

avatus Jan 23, 2025 •

edited

Loading

public-teleport-github-review-bot bot commented Jan 23, 2025

	// RoleDiffProps are an optonal set of props to render the role diff visualizer.
	// RoleDiffProps are an optional set of props to render the role diff visualizer.

Add optional component props to Roles component #51322

Add optional component props to Roles component #51322

Conversation

avatus commented Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanclark Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

avatus Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

public-teleport-github-review-bot bot commented Jan 23, 2025

avatus commented Jan 22, 2025 •

edited

Loading

ryanclark Jan 23, 2025 •

edited

Loading

avatus Jan 23, 2025 •

edited

Loading