- Notifications
You must be signed in to change notification settings - Fork1.1k
feat: create idp sync page skeleton#14543
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Parkreiner left a comment
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.
Noticed a couple of things, but I'm not going to block on them
I know there's going to be more polish, so I'll leave it to you to decide when these should be addressed
Uh oh!
There was an error while loading.Please reload this page.
| skip_issuer_checks:true, | ||
| }; | ||
| exportconstIdpSyncPage:FC=()=>{ |
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.
Just calling this out because I don't see anything about this in the commented-out code: do we want to do a redirect if the user navigates to this page iforganization.permissions.editMembers is false?
I know we're using that property to define whether we show the page link in the side navbar, but is there anything stopping someone from navigating to the page directly?
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.
@Parkreiner I haven't addressed permissions yet because a permission hasn't been defined for this page. Btw, how do other pages handle this case when the user doesn't have the permissions?
| organization_mapping:{}, | ||
| organization_assign_default:true, | ||
| group_auto_create:false, | ||
| group_regex_filter:"^Coder-.*$", |
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 know this is a mock value, but could I get more context on where this regex is used? Is it strictly server-controlled?
Wondering because the.*$ at the end literally does nothing in this case except make the regex run slower
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.
All the data is readonly and for now I just took the value from Steven. Im still learning myself how this feature works.
| exporttypeIdpSyncPageViewProps={ | ||
| oidcConfig:OIDCConfig|undefined; | ||
| }; |
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.
Since this is the only prop right now, do we want to couple the UI so tightly to the shape of the API response? My gut feeling is "no", but I don't know enough about the domain to know if this is going to matter much, especially if we have the mock config defined
Nit: I feel like we could also wrap this type in theReadonly utility type
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 actually don't know yet what the API response will look like. All I know is that it will have some similarity to what exists in OIDCConfig. So right now this is just meant to be an approximation to mock some data with the assumption that it will change.
site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpSyncPageView.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpSyncPageView.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpSyncPageView.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpSyncPageView.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpSyncPageView.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
BrunoQuaresma left a comment
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.
Since@aslilac and@Parkreiner are reviewing this as well I just did some QA and it looks ok considering the scope of this PR 👍
f6f18ce to1523025Compare
Uh oh!
There was an error while loading.Please reload this page.
resolves#14423
This is meant to be a first pass at the IdP Sync page while waiting for the backend work to be completed. Additional, polish and refinement will be handled in#14424