-
Notifications
You must be signed in to change notification settings - Fork 229
chore: remove Atlas Sign-In logic COMPASS-9595 #7150
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
base: main
Are you sure you want to change the base?
Conversation
Since we're removing the signin requirement from the AI features we do not hacve any components in need of the Atlas sign-in flows. As it's also built using a pattern we have deviated from, it's best to remove it now and re-add it through a different structure later if needed.
@@ -60,18 +53,6 @@ export function configureStore( | |||
) | |||
); | |||
|
|||
options.atlasAuthService.on('signed-in', () => { | |||
void store.dispatch(getUserInfo()); |
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.
sanity check: this isn't necessary for data explorer-related thing is it?
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.
On cloud we are always in logged in state, plus we don't have settings feature there.
I don't think we are now listening for these events anywhere else, so probably should be safe to clean that as well.
// because oidc options overlap with atlas login used for ai feature | ||
isAIFeatureEnabled | ||
) { | ||
if (isOIDCEnabled) { |
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.
if I understand right we still have OIDC logins(?). I'm not sure if any of the removed code may have been an unintentional dependency there.
With this change, would Compass AI features work with the current atlas apis? Or does this change need api endpoint updates and backend in production? |
Great, totally missed the target part in your comment. I see that you've removed |
@mabaasit I rename the environment variable to be about the AI opt-in, does that sound good? |
I added a feature flag now so we will only want to do this cleanup after we actually release the feature. |
We also have to wait for the new endpoints to be released to production and enabled, probably on staging and QA: https://wiki.corp.mongodb.com/spaces/MMS/pages/285087612/Config+Service+Application+Property+Admin+UI |
DO NOT MERGE UNTIL FEATURE FLAG IN #7129 IS MOVED TO RELEASED
Since we're removing the signin requirement from the AI features we do not hacve any components in need of the Atlas sign-in flows. As it's also built using a pattern we have deviated from, it's best to remove it now and re-add it through a different structure later if needed.
I'm relying on tests to ensure no unintentional functionality breaks but let me know if this has unintended effects on parts of Compass or DE that depend on auth for different reasons.