-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
base: master
Are you sure you want to change the base?
Conversation
export type IntegrationEnrollStepStatus = { | ||
code: IntegrationEnrollStatusCode; | ||
error?: string; | ||
}; |
There was a problem hiding this comment.
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?
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:
- The error is now required for the error status code.
- 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
)
There was a problem hiding this comment.
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
/** | ||
* AWSIC steps defined for AWS Idenity Center plugin. | ||
* Value matches with proto enums defined in the backend. | ||
*/ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I'm curious about mapping to integers instead of strings. That's based entirely on a single time where I had to do something similar in the Web UI, so I'm not sure if my assumptions are correct.
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', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Success = 'INTEGRATION_ENROLL_STATUS_CODE_SUCCESS', | ||
Skipped = 'INTEGRATION_ENROLL_STATUS_CODE_SKIPPED', | ||
Error = 'INTEGRATION_ENROLL_STATUS_CODE_ERROR', | ||
Aborted = 'INTEGRATION_ENROLL_STATUS_CODE_ABORTED', |
There was a problem hiding this comment.
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.
This PR:
INTEGRATION_ENROLL_KIND_AWS_IDENTITY_CENTER
toIntegrationEnrollKind
enum, which is used to identify integration types in the posthog events.Type values matches as defined in the proto type #51082
Part of https://github.com/gravitational/teleport.e/issues/5513