-
Notifications
You must be signed in to change notification settings - Fork 33
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
Retrieve and refresh integration insight on demand #499
Conversation
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.
LGTM - walked through changes with @etsai-stripe and everything makes sense 👍
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 had a couple questions but it looks awesome!
src/stripeWorkspaceState.ts
Outdated
if (error.code === 12) { | ||
// https://grpc.github.io/grpc/core/md_doc_statuscodes.html | ||
// 12: UNIMPLEMENTED |
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: the grpc library should have these error codes built in as constants.
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 will use the grpc constants
src/stripeWorkspaceState.ts
Outdated
vscode.window.showErrorMessage( | ||
'Please upgrade your Stripe CLI to the latest version to retrieve integration insight.', | ||
); |
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.
It looks like we're not resolving the promise in this case. Would this cause the log to load indefinitely?
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.
good catch!
// if insight has not been retrieved or previously failed to be retrieved, then retrieve it | ||
if (!('insight' in logDetails) || logDetails.insight.includes('Failed to retrieve insight') || logDetails.insight.includes('Please check back later')) { | ||
const insight = await getIntegrationInsight(logId, daemonClient); | ||
logDetails.insight = insight; |
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: I wonder if we should avoid setting this field if there's no insight. So the user won't see an empty string. What do you think?
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.
currently there could be empty insight. i am trying to distinguish between insight not retrieved yet
and actual empty insight
, so we dont keep retrieving insight for the same log.
This reverts commit 40a884a.
https://jira.corp.stripe.com/browse/DX-7061
dependent on PR stripe/stripe-cli#803
Integration insight retrieved on demand. Refreshed when log becomes available on ES
Integration insight retrieved successfully. Matches the insight from dashboard