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

Better errors #294

Merged
merged 10 commits into from
Jul 19, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
1 change: 1 addition & 0 deletions .github/workflows/test-meticulous-against-deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ on:
pull_request: {}

permissions:
actions: write
contents: read
statuses: write
pull-requests: write
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/test-meticulous.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ on:
pull_request: {}

permissions:
actions: write
contents: read
statuses: write
pull-requests: write
Expand Down
4 changes: 1 addition & 3 deletions src/action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { getEnvironment } from "./utils/environment.utils";
import { getBaseAndHeadCommitShas } from "./utils/get-base-and-head-commit-shas";
import { getCodeChangeEvent } from "./utils/get-code-change-event";
import { getInputs } from "./utils/get-inputs";
import { initLogger, setLogLevel } from "./utils/logger.utils";
import { initLogger, setLogLevel, shortSha } from "./utils/logger.utils";
import { ResultsReporter } from "./utils/results-reporter";
import { waitForDeploymentUrl } from "./utils/wait-for-deployment-url";

Expand Down Expand Up @@ -224,5 +224,3 @@ const getOctokitOrFail = (githubToken: string | null) => {
);
}
};

const shortSha = (sha: string) => sha.slice(0, 7);
2 changes: 2 additions & 0 deletions src/utils/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,5 @@ export const EXPECTED_PERMISSIONS_BLOCK = [
// the environment changes in a way that would cause a replay to behave differently, e.g. upgrading to a newer
// replay-orchestrator-launcher version, or changing the version of puppeteer.
export const LOGICAL_ENVIRONMENT_VERSION = 2;

export const DOCS_URL = "https://app.meticulous.ai/docs/github-actions-v2";
35 changes: 27 additions & 8 deletions src/utils/ensure-base-exists.utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ import { METICULOUS_LOGGER_NAME } from "@alwaysmeticulous/common";
import log from "loglevel";
import { Duration } from "luxon";
import { CodeChangeEvent } from "../types";
import { LOGICAL_ENVIRONMENT_VERSION } from "./constants";
import { DOCS_URL, LOGICAL_ENVIRONMENT_VERSION } from "./constants";
import {
DEFAULT_FAILED_OCTOKIT_REQUEST_MESSAGE,
isGithubPermissionsError,
} from "./error.utils";
import {
getCurrentWorkflowId,
getPendingWorkflowRun,
Expand Down Expand Up @@ -251,11 +255,26 @@ const getHeadCommitForRef = async ({
ref: string;
octokit: InstanceType<typeof GitHub>;
}): Promise<string> => {
const result = await octokit.rest.repos.getBranch({
owner,
repo,
branch: ref,
});
const commitSha = result.data.commit.sha;
return commitSha;
try {
const result = await octokit.rest.repos.getBranch({
owner,
repo,
branch: ref,
});
const commitSha = result.data.commit.sha;
return commitSha;
} catch (err: unknown) {
if (isGithubPermissionsError(err)) {
// https://docs.github.com/en/rest/overview/permissions-required-for-github-apps?apiVersion=2022-11-28#repository-permissions-for-contents
throw new Error(
`Missing permission to get the head commit of the branch '${ref}'. This is required in order to correctly calculate the two commits to compare.` +
` Please add the 'contents: read' permission to your workflow YAML file: see ${DOCS_URL} for the correct setup.`
);
}
const logger = log.getLogger(METICULOUS_LOGGER_NAME);
logger.error(
`Unable to get head commit of branch '${ref}'. This is required in order to correctly calculate the two commits to compare. ${DEFAULT_FAILED_OCTOKIT_REQUEST_MESSAGE}`
);
throw err;
}
};
29 changes: 29 additions & 0 deletions src/utils/error.utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { DOCS_URL } from "./constants";

export const isGithubPermissionsError = (
error: MaybeOctokitRequestError | unknown
): boolean => {
const message = getErrorMessage(error);
return (
message.toLowerCase().includes("resource not accessible by integration") ||
(error as MaybeOctokitRequestError)?.status === 403
);
};

export const getErrorMessage = (error: unknown) => {
const message = (error as MaybeOctokitRequestError)?.message ?? "";
return typeof message === "string" ? message : "";
};

// Octokit returns RequestErrors, which have a message and a code
// But we can't make any strong assumptions, so we type it on the safe side
// https://github.com/octokit/request-error.js/blob/372097e9b16f70d4ad75089003dc9154e304faa7/src/index.ts#L16
type MaybeOctokitRequestError =
| {
message?: string | unknown;
status?: number | unknown;
}
| null
| undefined;

export const DEFAULT_FAILED_OCTOKIT_REQUEST_MESSAGE = `Please check www.githubstatus.com, and that you have setup the action correctly, including with the correct permissions: see ${DOCS_URL} for the correct setup.`;
2 changes: 2 additions & 0 deletions src/utils/logger.utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,5 @@ export const setLogLevel: (logLevel: string | undefined) => void = (
break;
}
};

export const shortSha = (sha: string) => sha.slice(0, 7);
Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhat duplicative with the logic in our monorepo (source). Is it worth moving to the sdk or too small?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think too small to be worth it

49 changes: 37 additions & 12 deletions src/utils/results-reporter.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
import { getOctokit } from "@actions/github";
import { Project } from "@alwaysmeticulous/api";
import { METICULOUS_LOGGER_NAME } from "@alwaysmeticulous/common";
import {
ExecuteTestRunResult,
RunningTestRunExecution,
} from "@alwaysmeticulous/sdk-bundles-api";
import log from "loglevel";
import { CodeChangeEvent } from "../types";
import { DOCS_URL } from "./constants";
import {
DEFAULT_FAILED_OCTOKIT_REQUEST_MESSAGE,
isGithubPermissionsError,
} from "./error.utils";
import { shortSha } from "./logger.utils";
import { updateStatusComment } from "./update-status-comment";

const SHORT_SHA_LENGTH = 7;
Expand Down Expand Up @@ -160,18 +168,35 @@ export class ResultsReporter {
targetUrl?: string;
}) {
const { octokit, owner, repo, headSha } = this.options;
return octokit.rest.repos.createCommitStatus({
owner,
repo,
context:
this.options.testSuiteId != null
? `Meticulous (${this.options.testSuiteId})`
: "Meticulous",
description,
state,
sha: headSha,
...(targetUrl ? { target_url: targetUrl } : {}),
});
try {
return octokit.rest.repos.createCommitStatus({
owner,
repo,
context:
this.options.testSuiteId != null
? `Meticulous (${this.options.testSuiteId})`
: "Meticulous",
description,
state,
sha: headSha,
...(targetUrl ? { target_url: targetUrl } : {}),
});
} catch (err: unknown) {
if (isGithubPermissionsError(err)) {
// https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs
throw new Error(
`Missing permission to create and update commit statuses.` +
` Please add the 'statuses: write' permission to your workflow YAML file: see ${DOCS_URL} for the correct setup.`
);
}
const logger = log.getLogger(METICULOUS_LOGGER_NAME);
logger.error(
`Unable to create commit status for commit '${shortSha(
headSha
)}'. ${DEFAULT_FAILED_OCTOKIT_REQUEST_MESSAGE}`
);
throw err;
}
}

private setStatusComment({
Expand Down
74 changes: 48 additions & 26 deletions src/utils/update-status-comment.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
import { getOctokit } from "@actions/github";
import { METICULOUS_LOGGER_NAME } from "@alwaysmeticulous/common";
import log from "loglevel";
import { CodeChangeEvent } from "../types";
import { DOCS_URL } from "./constants";
import {
DEFAULT_FAILED_OCTOKIT_REQUEST_MESSAGE,
isGithubPermissionsError,
} from "./error.utils";

const getMeticulousCommentIdentifier = (testSuiteId: string | null) =>
`<!--- alwaysmeticulous/report-diffs-action/status-comment${
Expand Down Expand Up @@ -30,35 +37,50 @@ export const updateStatusComment = async ({
}

// Check for existing comments
const comments = await octokit.rest.issues.listComments({
owner,
repo,
issue_number: event.payload.pull_request.number,
per_page: 1000,
});
const commentIdentifier = getMeticulousCommentIdentifier(testSuiteId);
const existingComment = comments.data.find(
(comment) => (comment.body ?? "").indexOf(commentIdentifier) > -1
);
const testSuiteDescription = testSuiteId
? `Test suite: ${testSuiteId}. `
: "";

const fullBody = `${body}\n\n<sub>${testSuiteDescription}Last updated for commit ${shortHeadSha}. This comment will update as new commits are pushed.</sub>${commentIdentifier}`;

if (existingComment != null) {
await octokit.rest.issues.updateComment({
owner,
repo,
comment_id: existingComment.id,
body: fullBody,
});
} else if (createIfDoesNotExist) {
await octokit.rest.issues.createComment({
try {
const comments = await octokit.rest.issues.listComments({
owner,
repo,
issue_number: event.payload.pull_request.number,
body: fullBody,
per_page: 1000,
});

const commentIdentifier = getMeticulousCommentIdentifier(testSuiteId);
const existingComment = comments.data.find(
(comment) => (comment.body ?? "").indexOf(commentIdentifier) > -1
);
const testSuiteDescription = testSuiteId
? `Test suite: ${testSuiteId}. `
: "";

const fullBody = `${body}\n\n<sub>${testSuiteDescription}Last updated for commit ${shortHeadSha}. This comment will update as new commits are pushed.</sub>${commentIdentifier}`;

if (existingComment != null) {
await octokit.rest.issues.updateComment({
owner,
repo,
comment_id: existingComment.id,
body: fullBody,
});
} else if (createIfDoesNotExist) {
await octokit.rest.issues.createComment({
owner,
repo,
issue_number: event.payload.pull_request.number,
body: fullBody,
});
}
} catch (err) {
if (isGithubPermissionsError(err)) {
// https://docs.github.com/en/rest/overview/permissions-required-for-github-apps?apiVersion=2022-11-28#repository-permissions-for-pull-requests
throw new Error(
`Missing permission to list and post comments to the pull request #${event.payload.pull_request.number}. Please add the 'pull-requests: write' permission to your workflow YAML file: see ${DOCS_URL} for the correct setup.`
);
}
const logger = log.getLogger(METICULOUS_LOGGER_NAME);
logger.error(
`Unable to post / update comment on PR #${event.payload.pull_request.number}. ${DEFAULT_FAILED_OCTOKIT_REQUEST_MESSAGE}`
);
throw err;
}
};
15 changes: 12 additions & 3 deletions src/utils/wait-for-deployment-url.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import { getOctokit } from "@actions/github";
import { GitHub } from "@actions/github/lib/utils";
import { METICULOUS_LOGGER_NAME } from "@alwaysmeticulous/common";

Check warning on line 3 in src/utils/wait-for-deployment-url.ts

View workflow job for this annotation

GitHub Actions / Lint, format and test

'METICULOUS_LOGGER_NAME' is defined but never used
Que3216 marked this conversation as resolved.
Show resolved Hide resolved
import { Hub } from "@sentry/node";
import { Transaction } from "@sentry/types";
import { EXPECTED_PERMISSIONS_BLOCK } from "./constants";
import log from "loglevel";

Check warning on line 6 in src/utils/wait-for-deployment-url.ts

View workflow job for this annotation

GitHub Actions / Lint, format and test

'log' is defined but never used
import { DOCS_URL, EXPECTED_PERMISSIONS_BLOCK } from "./constants";

Check warning on line 7 in src/utils/wait-for-deployment-url.ts

View workflow job for this annotation

GitHub Actions / Lint, format and test

'DOCS_URL' is defined but never used
import { isGithubPermissionsError } from "./error.utils";

const TIMEOUT_MS = 30 * 60 * 1_000; // 30 minutes
const MIN_POLL_FREQUENCY = 1_000;
Expand Down Expand Up @@ -101,15 +104,21 @@
per_page: MAX_GITHUB_ALLOWED_PAGE_SIZE,
});
} catch (err) {
console.error("Error listing deployments", err);
if (!isGithubPermissionsError(err)) {
// If it's a permission error our main error message is sufficient, we don't need to log the raw github one,
// but otherwise we should log it:
console.error("Error listing deployments", err);
}
}

if (deployments == null || deployments.status !== 200) {
// https://docs.github.com/en/rest/overview/permissions-required-for-github-apps?apiVersion=2022-11-28#repository-permissions-for-deployments
throw new Error(
`Failed to list deployments for commit ${commitSha}.\n\n` +
"Note: if using 'use-deployment-url' then you must provide permissions for the action to read deployments. " +
"To do this edit the 'permissions:' block in your workflow file to include 'deployments: read'. Your permissions block should look like:\n\n" +
EXPECTED_PERMISSIONS_BLOCK
EXPECTED_PERMISSIONS_BLOCK +
"\n\nSee ${DOCS_URL} for the correct setup."
Que3216 marked this conversation as resolved.
Show resolved Hide resolved
);
}

Expand Down
57 changes: 51 additions & 6 deletions src/utils/workflow.utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ import { GitHub } from "@actions/github/lib/utils";
import { METICULOUS_LOGGER_NAME } from "@alwaysmeticulous/common";
import log from "loglevel";
import { DateTime, Duration } from "luxon";
import { DOCS_URL } from "./constants";
import { isGithubPermissionsError } from "./error.utils";
import { shortSha } from "./logger.utils";

// The GitHub REST API will not list a workflow run immediately after it has been dispatched
const LISTING_AFTER_DISPATCH_DELAY = Duration.fromObject({ seconds: 10 });
Expand Down Expand Up @@ -42,12 +45,54 @@ export const startNewWorkflowRun = async ({
commitSha: string;
octokit: InstanceType<typeof GitHub>;
}): Promise<{ workflowRunId: number; [key: string]: unknown } | undefined> => {
await octokit.rest.actions.createWorkflowDispatch({
owner,
repo,
workflow_id: workflowId,
ref,
});
try {
await octokit.rest.actions.createWorkflowDispatch({
owner,
repo,
workflow_id: workflowId,
ref,
});
} catch (err: unknown) {
const logger = log.getLogger(METICULOUS_LOGGER_NAME);
const message = (err as { message?: string } | null)?.message ?? "";
if (
message.includes("Workflow does not have 'workflow_dispatch' trigger")
) {
logger.error(
`Could not trigger a workflow run on commit ${shortSha(
commitSha
)} of the base branch (${ref}) to compare against, because there was no Meticulous workflow with the 'workflow_dispatch' trigger on the ${ref} branch.` +
` Screenshots of the new flows will be taken, but no comparisons will be made.` +
` If you haven't merged the PR to setup Meticulous in Github Actions to the ${ref} branch yet then this is expected.` +
` Otherwise please check that Meticulous is running on the ${ref} branch, that it has a 'workflow_dispatch' trigger, and has the appropiate permissions.` +
` See ${DOCS_URL} for the correct setup.`
);
logger.debug(err);
return undefined;
}
if (isGithubPermissionsError(err)) {
// https://docs.github.com/en/rest/overview/permissions-required-for-github-apps?apiVersion=2022-11-28#repository-permissions-for-actions
logger.error(
`Missing permission to trigger a workflow run on the base branch (${ref}).` +
` Screenshots of the new flows will be taken, but no comparisons will be made.` +
` Please add the 'actions: write' permission to your workflow YAML file: see ${DOCS_URL} for the correct setup.`
);
logger.debug(err);
return undefined;
}

logger.error(
`Could not trigger a workflow run on commit ${shortSha(
commitSha
)} of the base branch (${ref}) to compare against.` +
` Screenshots of the new flows will be taken, but no comparisons will be made.` +
` Please check that Meticulous is running on the ${ref} branch, that it has a 'workflow_dispatch' trigger, and has the appropiate permissions.` +
` See ${DOCS_URL} for the correct setup.`,
err
);
return undefined;
}

// Wait before listing again
await delay(LISTING_AFTER_DISPATCH_DELAY);

Expand Down
Loading