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

web: define integration enroll step event types #51097

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

web: define integration enroll step event types #51097

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02b8758
define integration enroll step event types
flyinghermit Jan 16, 2025
ade358d
use discriminating union to narrow down error type and optional message
flyinghermit Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 61 additions & 10 deletions web/packages/teleport/src/services/userEvent/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,14 @@ export enum CaptureEvent {
OnboardQuestionnaireSubmitEvent = 'tp.ui.onboard.questionnaire.submit',
}

/**
* IntegrationEnrollEvent defines integration enrollment
* events.
*/
export enum IntegrationEnrollEvent {
Started = 'tp.ui.integrationEnroll.start',
Complete = 'tp.ui.integrationEnroll.complete',
Step = 'tp.ui.integrationEnroll.step',
}

// IntegrationEnrollKind represents a integration type.
Expand Down Expand Up @@ -76,8 +81,64 @@ export enum IntegrationEnrollKind {
MachineIDKubernetes = 'INTEGRATION_ENROLL_KIND_MACHINE_ID_KUBERNETES',
EntraId = 'INTEGRATION_ENROLL_KIND_ENTRA_ID',
DatadogIncidentManagement = 'INTEGRATION_ENROLL_KIND_DATADOG_INCIDENT_MANAGEMENT',
AwsIdentityCenter = 'INTEGRATION_ENROLL_KIND_AWS_IDENTITY_CENTER',
}

/**
* IntegrationEnrollStep defines configurable steps for an integration type.
*/
export enum IntegrationEnrollStep {
/**
* AWSIC steps defined for AWS Idenity Center plugin.
* Value matches with proto enums defined in the backend.
*/
Comment on lines +91 to +94
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment should be placed in a line above.

ConnectOidc = 'INTEGRATION_ENROLL_STEP_AWSIC_CONNECT_OIDC',
ImportResourceSetDefaultOwner = 'INTEGRATION_ENROLL_STEP_AWSIC_SET_ACCESSLIST_DEFAULT_OWNER',
IdentitySourceUploadSamlMetadata = 'INTEGRATION_ENROLL_STEP_AWSIC_UPLOAD_AWS_SAML_SP_METADATA',
ScimTestConnection = 'INTEGRATION_ENROLL_STEP_AWSIC_TEST_SCIM_CONNECTION',
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't you want these to match enum values instead, as in integers? I assume the web API is going to return those integers, not string values, maybe that assumption is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did such integer definition when I previously added events but after working in the discover components, I appreciated the shorter name by defining it this way IntegrationEnrollStep.ConnectOidc vs IntegrationEnrollStep.INTEGRATION_ENROLL_STEP_AWSIC_CONNECT_OIDC . The conversion to the integer happens in the web api where we convert it to the proto type.

Though the best way to do is to definitely defining exact 1:1 enum name and value. If this was auto generated *.pb.ts, I would have lived with the longer name but hand defining them lets us do some conversion in the backend and save us from looong name.

}

/**
* IntegrationEnrollStatusCode defines status codes for a given
* integration configuration step event.
* Value matches with proto enums defined in the backend.
*/
export enum IntegrationEnrollStatusCode {
Success = 'INTEGRATION_ENROLL_STATUS_CODE_SUCCESS',
Skipped = 'INTEGRATION_ENROLL_STATUS_CODE_SKIPPED',
Error = 'INTEGRATION_ENROLL_STATUS_CODE_ERROR',
Aborted = 'INTEGRATION_ENROLL_STATUS_CODE_ABORTED',
Copy link
Member

Choose a reason for hiding this comment

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

Same here wrt to how this is mapped to the proto enum.

}

/**
* IntegrationEnrollStepStatus defines fields for reporting
* integration configuration step event.
*/
export type IntegrationEnrollStepStatus = {
code: IntegrationEnrollStatusCode;
error?: string;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

What would you say for defining this type as a discriminated union?

Suggested change
export type IntegrationEnrollStepStatus = {
code: IntegrationEnrollStatusCode;
error?: string;
};
export type IntegrationEnrollStepStatus =
| {
code: Exclude<
IntegrationEnrollStatusCode,
IntegrationEnrollStatusCode.Error
>;
}
| {
code: IntegrationEnrollStatusCode.Error;
error: string;
};

This has some benefits over the optional field:

  1. The error is now required for the error status code.
  2. It prevents passing the error for non-error status codes.

Then I'd change the emitEvent signature to:

export function emitEvent(
  eventId: string,
  step: IntegrationEnrollStep,
  status: IntegrationEnrollStepStatus
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it + makes the input param types stronger. Many thanks for the suggestion ade358d


/**
* IntegrationEnrollEventData defines integration
* enroll event. Use for start, complete and step events.
*/
export type IntegrationEnrollEventData = {
id: string;
kind: IntegrationEnrollKind;
step?: IntegrationEnrollStep;
status?: IntegrationEnrollStepStatus;
};

/**
* IntegrationEnrollEventRequest defines integration enroll
* event request as expected in the backend.
*/
export type IntegrationEnrollEventRequest = {
event: IntegrationEnrollEvent;
eventData: IntegrationEnrollEventData;
};

// These constants should match the constant defined in backend found in:
// lib/usagereporter/web/userevent.go
export enum DiscoverEvent {
Expand Down Expand Up @@ -177,16 +238,6 @@ export type EventMeta = {

export type PreUserEvent = UserEvent & EventMeta;

export type IntegrationEnrollEventData = {
id: string;
kind: IntegrationEnrollKind;
};

export type IntegrationEnrollEventRequest = {
event: IntegrationEnrollEvent;
eventData: IntegrationEnrollEventData;
};

export type DiscoverEventRequest = Omit<UserEvent, 'event'> & {
event: DiscoverEvent;
eventData: DiscoverEventData;
Expand Down
Loading