Alter readiness check to use the is_ready API endpoint #278
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
blindly trying the application frontend.
There was a problem hiding this comment.
looks good in general. couple notes:
- fetchOptions should get passed as the second arg of the axios.get, right now it's not being used.
- Also see that there's already a
getAppReadymethod in the API. You should just replace that one with the new one, and make sure it's still working wherever the existing method was being used. Also make sure to update thegetAppReadydeclaration in the IWorkspacesAPI interface, or you'll get errors. Also, just return a boolean, not an object with anis_readyproperty. Part of the role of the API is to parse the raw API response into something more easily useable.
…o use new endpoint
|
updated to conform to suggestions |
Looks good, only gripe is the api call should be try caught by the user, not inside the call itself. Putting it inside the api call can mess up some of the error handling |
|
Hmm yeah true, but I'll also say that in both of the calling contexts where it is invoked, the code treats an error the same as false and simply blindly retries the call after a pause. |
|
Yeah but it’s about code consistency. And also like I said, the error handling in the code for any APIRequest method breaks if you catch errors inside the request, so it literally breaks the code in edge cases to do that
Get Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: Jeff Waller ***@***.***>
Sent: Thursday, October 12, 2023 11:05:45 AM
To: helxplatform/helx-ui ***@***.***>
Cc: Roupe, Griffin ***@***.***>; Review requested ***@***.***>
Subject: Re: [helxplatform/helx-ui] Alter readiness check to use the is_ready API endpoint (PR #278)
Hmm yeah true, but I'll also say that in both of the calling contexts where it is invoked, the code treats an error the same as false and simply blindly retries the call after a pause.
—
Reply to this email directly, view it on GitHub<#278 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AHRKQOLHZBASASEJTY7OCO3X7ABMTAVCNFSM6AAAAAA52Y2WLOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJZG44TGMRRG4>.
You are receiving this because your review was requested.Message ID: ***@***.***>
|
|
modded the try-catches too. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add support for checking the readiness of an App in a different way. Previously, the UI (splash screen) would try to
connect the main interface of the App. With this change, it instead checks against a new API endpoint.
/v1/instances/{sid}/is_ready/. The result will be false until the pod underlying the App passes its readiness probe.