-
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
Okta: Backend/type changes for Okta Integration enrolment improvements #51731
base: master
Are you sure you want to change the base?
Conversation
c1d9dea
to
c34e938
Compare
c34e938
to
f2b7a4e
Compare
@@ -32,6 +32,8 @@ import ( | |||
const ( | |||
// OIDCJWKWURI is the relative path where the OIDC IdP JWKS is located | |||
OIDCJWKWURI = "/.well-known/jwks-oidc" | |||
// OktaJWKSURI is the relative path where the Okta JWKS is located |
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.
lol, is OIDCJWKWURI
is a typo? (has a W instead of a S)
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.
looks like it, I just didn't want to increase the scope of this Okta work further by updating it
[key: string]: any; | ||
}; | ||
|
||
export type PluginNameToDetails = { | ||
okta: PluginStatusOkta; | ||
[key: string]: any; |
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.
do we need the [key: string]: any;
?
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 added these for using w/ some pluginsService
funcs, so specifying one of those name
s will result in the return being properly typed... Since the default for the Plugin
type's Spec
/Status
fields with no generics provided is any
I figured I'd just keep that the same. Could remove it though and be a bit stricter.
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.
+1 on removing here and above unless absolutely necessary.
@@ -32,6 +32,8 @@ import ( | |||
const ( | |||
// OIDCJWKWURI is the relative path where the OIDC IdP JWKS is located | |||
OIDCJWKWURI = "/.well-known/jwks-oidc" | |||
// OktaJWKSURI is the relative path where the Okta JWKS is located | |||
OktaJWKSURI = "/.well-known/jwks-okta" |
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.
nit: spell out what the URI refers to
OktaJWKSURI = "/.well-known/jwks-okta" | |
OktaJWKSWellknownURI = "/.well-known/jwks-okta" |
getIntegrationStatusRoute( | ||
type: PluginKind | IntegrationKind, | ||
name: string, | ||
page?: 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.
nit: from the PR description, I think we are referring to status page here correct?
page?: string | |
statusPage?: string |
| PluginDatadogSpec | ||
| PluginEmailSpec | ||
| PluginMsTeamsSpec; | ||
export type PluginNameToSpec = { |
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.
nit: JSDoc
[key: string]: any; | ||
}; | ||
|
||
export type PluginNameToDetails = { |
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.
nit: JSDoc
[key: string]: any; | ||
}; | ||
|
||
export type PluginNameToDetails = { | ||
okta: PluginStatusOkta; | ||
[key: string]: any; |
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.
+1 on removing here and above unless absolutely necessary.
credentialsInfo?: CredentialsInfo; | ||
}; | ||
|
||
export type CredentialsInfo = { |
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.
Can all fields be true at the same time?
JSDoc please.
Add types and update integration status page + enrol paths for Okta Integration enrolment changes. Required for #5994.