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.
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
feat: return runtime versions used by the application with a doctor hook #81
feat: return runtime versions used by the application with a doctor hook #81
Changes from 8 commits
24d4cda
936dcf7
5de5669
a580ff3
a8f80e7
e2385b9
8cf422a
1f28a44
fea6cef
d8e574d
be8acb7
f5429fe
96374f3
6045d51
c5c167c
30ce796
b81f76f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This can throw if the hostname can't be found. I hope that's rare because it doesn't seem obvious how we could catch this...
Here's the stacktrace:
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.
Let's add the error details here when we raise this, at least the HTTP response code.
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.
Included the status code here to hint at the error, but let me know if you think more would be better!
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.
In the same vein as my last comment, perhaps we pump the error details over the CLI/SDK protocol's diagnostic line to allow the CLI to more gracefully handle showing the error in the right place (high level error in the output, with a pointer to the debug log, where the full details can be shown)? WDYT?
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.
Let's add the
metadata.cli
response payload as part of the error too.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 call. I think this is useful, even if no obvious remediation exists
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.
Hmm, thoughts on maybe pumping the specific error to the debug log and not messing up the delicate doctor template? We may have to use the
protocol
's differingrespond
vs.log
methods to enable the CLI to distinguish between the two.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'm not sure if the deno-runtime used by run on slack infra is the "minimum" version I think it might just be the version being used since we did not implement a way for developers to set a specific version they can't change it
For example if a developer is using new features found in Deno 1.41.x and run on slack support 1.37.x that does not have those features then the developers code would fail so 1.37.x is not the minimum version
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.
Yeah we discussed this point in the spec @zimeg wrote. The
minimum
is just the payload shape, but you can see in the messaging to the user we don't actually mention minimum. This is intentional. We are very carefully choosing our words when raising this to the user so that we don't imply a 'minimum' version but rather communicate that ROSI uses one EXACT version.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 agree that "minimum" isn't a super ideal word for this case but it gives us flexibility across other SDKs to specify certain runtime requirements IMO.
With the
current
andminimum
version comparison moving into the hooks I'm super interested in how this output might change. Or maybe it doesn't? But wanted to share an example for reference: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.
Without special error handling in the CLI, the hook can output this messaging which I think is pretty alright! All of this info is coming from the hook.
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
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 to circle back on this discussion with the latest updates-
minimum
no longer exists. Neither doeslatest
. At least in the hook response. Onlymessage
anderrors.message
can provide additional detail.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 also 👍
The hook can also use the protocol to share an error message correct?
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.
To my earlier comments, can we check to make sure that the HTTP response code/body are at a minimum raised if the error caught is one of the HTTP or payload-parsing code fails? Perhaps add that as part of the test suite (if it's not already there).
Check warning on line 37 in src/doctor.ts
Codecov / codecov/patch
src/doctor.ts#L36-L37
Check warning on line 66 in src/doctor.ts
Codecov / codecov/patch
src/doctor.ts#L63-L66