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

Jump to bottom

fix: getkubernetesResourceManagers popup #9722

Closed
wants to merge 1 commit into from
Closed

fix: getkubernetesResourceManagers popup #9722

wants to merge 1 commit into from

Conversation

amandavialva01
Copy link
Contributor

@amandavialva01 amandavialva01 commented Jul 24, 2024 •
edited by jira bot
Loading

Ticket

Description

Fix getKubernetesResourceManager toast so that it doesn't show up on login screen after a user logs out.

Test Plan

CI passes.

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

@amandavialva01 amandavialva01 requested a review from a team as a code owner July 24, 2024 21:30
@amandavialva01 amandavialva01 requested a review from hkang1 July 24, 2024 21:30
@cla-bot cla-bot bot added the cla-signed label Jul 24, 2024
@netlify Netlify
Copy link

netlify bot commented Jul 24, 2024 •
edited
Loading

✅ Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit a5e27f4
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66a177cf2b59a9000818d4de

@amandavialva01 amandavialva01 force-pushed the fixGetK8sRMPopup branch 2 times, most recently from 83719d9 to 2b0c919 Compare July 24, 2024 21:50
@amandavialva01 amandavialva01 requested review from ashtonG and removed request for keita-determined July 24, 2024 21:50
@amandavialva01
fix: getkubernetesResourceManagers popup
a5e27f4 
Co-authored-by: name saloni.gupta@hpe.com
hkang1
hkang1 reviewed Jul 25, 2024
View reviewed changes
Copy link
Contributor

@hkang1 hkang1 left a comment

Choose a reason for hiding this comment

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

It's been a while since I've been in this code base, but had some questions on the changes made 🙏

webui/react/src/components/WorkspaceCreateModal.tsx
@@ -61,7 +61,7 @@ const isNonK8RMError = (e: unknown): boolean => {
};

const isNotAuthorizedErr = (e: unknown): boolean => {
return e instanceof DetError && e.sourceErr instanceof Response && e.sourceErr['status'] === 403;
return e instanceof DetError && e.sourceErr instanceof Response && (e.sourceErr['status'] === 403 || e.sourceErr['status'] === 401);
Copy link
Contributor

Choose a reason for hiding this comment

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

question:

  • is there enough overlap of this and isAuthFailure?
  • not for this PR but maybe this function can be made reusable into src/utils/services.ts if this is deemed common enough of a situation?

webui/react/src/components/WorkspaceCreateModal.tsx
@@ -110,7 +110,7 @@ const WorkspaceCreateModalComponent: React.FC<Props> = ({ onClose, workspaceId }
handleError(e, {
level: ErrorLevel.Error,
publicMessage: 'Failed to fetch list of workspace namespace bindings.',
silent: false,
silent: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

question: this effectively silences this error for good from the user perspective but will still get logged, confirming that it is what we want (as opposed to conditionally showing it to the user)

Copy link
Contributor Author

@amandavialva01 amandavialva01 Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The issue of logging errors from the getKubernetesResourceManagers API request I believe is resolved in a different PR. @salonig23 can you please link that PR here?

@ashtonG
Copy link
Contributor

ashtonG commented Jul 29, 2024

is this still necessary with #9735?

@salonig23
Copy link
Contributor

is this still necessary with #9735?

No it is not. @amandavialva01 can you close this PR?

@amandavialva01
Copy link
Contributor Author

is this still necessary with #9735?

No it is not. @amandavialva01 can you close this PR?

Yea I can close this, is there a PR where the issue of the getKubernetesResourceManagers toast gets resolved that I can link in the closing comment @salonig23?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hkang1 hkang1 hkang1 left review comments

@ashtonG ashtonG Awaiting requested review from ashtonG

Assignees
No one assigned
Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amandavialva01 @ashtonG @salonig23 @hkang1