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.

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

Merged
merged 17 commits into from
Mar 26, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,16 @@ invoked.

## Supported Scripts

The hooks currently provided by this repo are `build`, `start`, `check-update`,
`install-update`, `get-trigger`, and `get-manifest`.
The hooks currently provided by this repo are `build`, `check-update`, `doctor`,
`get-hooks`, `get-manifest`, `get-trigger`, `install-update`, and `start`.

| Hook Name | CLI Command | Description |
| ---------------- | ---------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| `build` | `slack deploy` | Bundles any functions with Deno into an output directory that's compatible with the Run on Slack runtime. Implemented in `build.ts`. |
| `check-update` | `slack upgrade` | Checks the App's SDK dependencies to determine whether or not any of your libraries need to be updated. Implemented in `check_update.ts`. |
| `get-manifest` | `slack manifest` | Converts a `manifest.json`, `manifest.js`, or `manifest.ts` file into a valid manifest JSON payload. Implemented in `get_manifest.ts`. |
| `doctor` | `slack doctor` | Returns runtime versions and other system dependencies required by the application. Implemented in `doctor.ts`. |
| `get-hooks` | All | Fetches the list of available hooks for the CLI from this repository. Implemented in `mod.ts`. |
| `get-manifest` | `slack manifest` | Converts a `manifest.json`, `manifest.js`, or `manifest.ts` file into a valid manifest JSON payload. Implemented in `get_manifest.ts`. |
| `get-trigger` | `slack trigger create` | Converts a specified `json`, `js`, or `ts` file into a valid trigger JSON payload to be uploaded by the CLI to the `workflows.triggers.create` Slack API endpoint. Implemented in `get_trigger.ts`. |
| `install-update` | `slack upgrade` | Prompts the user to automatically update any dependencies that need to be updated based on the result of the `check-update` hook. Implemented in `install_update.ts`. |
| `start` | `slack run` | While developing and locally running a deno-slack-based application, the CLI manages a socket connection with Slack's backend and delegates to this hook for invoking the correct application function for relevant events incoming via this connection. For more information, see the [deno-slack-runtime](https://github.com/slackapi/deno-slack-runtime) repository's details on `local-run`. |
Expand Down
66 changes: 66 additions & 0 deletions src/doctor.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { getProtocolInterface } from "./deps.ts";

type RuntimeVersion = {
name: string;
current: string;
minimum?: string;
error?: {
message: string;
};
};

const getHostedDenoRuntimeVersion = async (): Promise<{
minimum?: string;
zimeg marked this conversation as resolved.
Show resolved Hide resolved
message?: string;
error?: { message: string };
}> => {
try {
const metadataURL = "https://api.slack.com/slackcli/metadata.json";
WilliamBergamin marked this conversation as resolved.
Show resolved Hide resolved
const response = await fetch(metadataURL);
Copy link
Member Author

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...

Screenshot 2024-03-22 at 3 27 33 PM

Here's the stacktrace:

Screenshot 2024-03-22 at 3 28 22 PM

if (!response.ok) {
throw new Error("Failed to collect upstream CLI metadata");
Copy link

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.

Copy link
Member Author

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!

status

Copy link

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?

}
const metadata = await response.json();
const version = metadata?.["deno-runtime"]?.releases[0]?.version;
if (!version) {
throw new Error("Failed to find the minimum Deno version");
Copy link

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.

Copy link
Member Author

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

metadata-response

Copy link

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 differing respond vs. log methods to enable the CLI to distinguish between the two.

}
const message = Deno.version.deno !== version
? `Applications deployed to Slack use Deno version ${version}`
: undefined;
return { minimum: version, message };
Copy link
Contributor

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

Copy link

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.

Copy link
Member Author

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 and minimum 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:

minimum-error

Copy link
Member Author

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.

unsupported-runtime

Copy link

Choose a reason for hiding this comment

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

LGTM

Copy link
Member Author

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 does latest. At least in the hook response. Only message and errors.message can provide additional detail.

Copy link
Contributor

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?

} catch (err) {
if (err instanceof Error) {
return { error: { message: err.message } };
Copy link

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).

}
return { error: { message: err } };
}

Check warning on line 37 in src/doctor.ts

View check run for this annotation

Codecov / codecov/patch

src/doctor.ts#L36-L37

Added lines #L36 - L37 were not covered by tests
};

export const getRuntimeVersions = async (): Promise<{
versions: RuntimeVersion[];
}> => {
const hostedDenoRuntimeVersion = await getHostedDenoRuntimeVersion();
const versions = [
{
"name": "deno",
"current": Deno.version.deno,
...hostedDenoRuntimeVersion,
},
{
"name": "typescript",
"current": Deno.version.typescript,
},
{
"name": "v8",
"current": Deno.version.v8,
},
];
return { versions };
};

if (import.meta.main) {
const protocol = getProtocolInterface(Deno.args);
const prunedDoctor = await getRuntimeVersions();
protocol.respond(JSON.stringify(prunedDoctor));
}

Check warning on line 66 in src/doctor.ts

View check run for this annotation

Codecov / codecov/patch

src/doctor.ts#L63-L66

Added lines #L63 - L66 were not covered by tests
2 changes: 2 additions & 0 deletions src/mod.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ export const projectScripts = (args: string[]) => {
`deno run -q --config=deno.jsonc --allow-read --allow-net https://deno.land/x/${HOOKS_TAG}/check_update.ts`,
"install-update":
`deno run -q --config=deno.jsonc --allow-run --allow-read --allow-write --allow-net https://deno.land/x/${HOOKS_TAG}/install_update.ts`,
"doctor":
`deno run -q --config=deno.jsonc --allow-read --allow-net https://deno.land/x/${HOOKS_TAG}/doctor.ts`,
},
"config": {
"protocol-version": ["message-boundaries"],
Expand Down
162 changes: 162 additions & 0 deletions src/tests/doctor_test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
import { assertEquals } from "../dev_deps.ts";
import { mockFetch } from "../dev_deps.ts";
import { getRuntimeVersions } from "../doctor.ts";

const REAL_DENO_VERSION = Deno.version;
const MOCK_DENO_VERSION = {
deno: "1.2.3",
typescript: "5.0.0",
v8: "12.3.456.78",
};

const MOCK_SLACK_CLI_MANIFEST = {
"slack-cli": {
"title": "Slack CLI",
"description": "CLI for creating, building, and deploying Slack apps.",
"releases": [
{
"version": "2.19.0",
"release_date": "2024-03-11",
},
],
},
"deno-runtime": {
"title": "Deno Runtime",
"releases": [
{
"version": "1.101.1",
"release_date": "2023-09-19",
},
],
},
};

Deno.test("doctor hook tests", async (t) => {
Object.defineProperty(Deno, "version", {
value: MOCK_DENO_VERSION,
writable: true,
configurable: true,
});
mockFetch.install();

await t.step("known runtime values for the system are returned", async () => {
mockFetch.mock("GET@/slackcli/metadata.json", (_req: Request) => {
return new Response(null, { status: 404 });
});
Deno.version.deno = "1.2.3";

const actual = await getRuntimeVersions();
const expected = {
versions: [
{
name: "deno",
current: "1.2.3",
error: {
message: "Failed to collect upstream CLI metadata",
},
},
{
name: "typescript",
current: "5.0.0",
},
{
name: "v8",
current: "12.3.456.78",
},
],
};
assertEquals(actual, expected);
});

await t.step("matched upstream requirements return success", async () => {
mockFetch.mock("GET@/slackcli/metadata.json", (_req: Request) => {
return new Response(JSON.stringify(MOCK_SLACK_CLI_MANIFEST));
});
Deno.version.deno = "1.101.1";

const actual = await getRuntimeVersions();
const expected = {
versions: [
{
name: "deno",
current: "1.101.1",
minimum: "1.101.1",
message: undefined,
},
{
name: "typescript",
current: "5.0.0",
},
{
name: "v8",
current: "12.3.456.78",
},
],
};
assertEquals(actual, expected);
});

await t.step("mismatched upstream requirements note difference", async () => {
mockFetch.mock("GET@/slackcli/metadata.json", (_req: Request) => {
return new Response(JSON.stringify(MOCK_SLACK_CLI_MANIFEST));
});
Deno.version.deno = "1.2.3";

const actual = await getRuntimeVersions();
const expected = {
versions: [
{
name: "deno",
current: "1.2.3",
minimum: "1.101.1",
message: "Applications deployed to Slack use Deno version 1.101.1",
},
{
name: "typescript",
current: "5.0.0",
},
{
name: "v8",
current: "12.3.456.78",
},
],
};
assertEquals(actual, expected);
});

await t.step("missing minimums from cli metadata are noted", async () => {
mockFetch.mock("GET@/slackcli/metadata.json", (_req: Request) => {
return new Response(JSON.stringify({}));
});
Deno.version.deno = "1.2.3";

const actual = await getRuntimeVersions();
const expected = {
versions: [
{
name: "deno",
current: "1.2.3",
error: {
message: "Failed to find the minimum Deno version",
},
},
{
name: "typescript",
current: "5.0.0",
},
{
name: "v8",
current: "12.3.456.78",
},
],
};
assertEquals(actual, expected);
});

Object.defineProperty(Deno, "version", {
value: REAL_DENO_VERSION,
writable: false,
configurable: false,
});
mockFetch.uninstall();
});
Loading