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

Better errors #294

merged 10 commits into from
Jul 19, 2023

Conversation

Que3216
Copy link
Contributor

@Que3216 Que3216 commented Jul 17, 2023

image image

@vercel
Copy link

vercel bot commented Jul 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
report-diffs-action ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 19, 2023 2:12pm

@github-actions
Copy link

github-actions bot commented Jul 17, 2023

✅ Meticulous spotted zero visual differences across 8 screens tested: view results.

Test suite: test Meticulous with app url. Last updated for commit ce50df6. This comment will update as new commits are pushed.

@github-actions
Copy link

github-actions bot commented Jul 17, 2023

🤖 Meticulous replayed 1 user sessions and took 8 screenshots. Meticulous did not run on main of the main branch and so there was nothing to compare against.

Please merge your pull request for setting up Meticulous in CI and ensure that it’s running on push events to the main branch.

Test suite: test Meticulous with deployment url. Last updated for commit ce50df6. This comment will update as new commits are pushed.

const logger = log.getLogger(METICULOUS_LOGGER_NAME);
if (
(err as { message?: string } | null)?.message?.includes(
"Workflow does not have 'workflow_dispatch' trigger"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error returned is a 422 returned by GH servers (https://github.com/octokit/request-error.js/blob/372097e9b16f70d4ad75089003dc9154e304faa7/src/index.ts#L16):

RequestError [HttpError]: Workflow does not have 'workflow_dispatch' trigger
     at /app/node_modules/@octokit/request/dist-node/index.js:86:21
     at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
     at async $1e5661a16c61ad2e$export$e6c4ffd711240397 (file:///app/dist/index.mjs:291:5)
     at async $9bad9e90aadb16eb$export$fe64d28a00d9670b (file:///app/dist/index.mjs:474:25)
     at async $9bad9e90aadb16eb$export$246429c52b3b3bd3 (file:///app/dist/index.mjs:367:16)
     at async $73105851ee1dace6$export$4196946125a2d37f (file:///app/dist/index.mjs:1029:59) {
   status: 422,
   response: {
     url: <redacted>
     status: 422,
...

It has no more precise error code than that, so have to use the message unfortunately. If they change this message it'll just fall through to the more general case below, so not the end of the world.

Copy link
Contributor

@dbook13 dbook13 left a comment

Choose a reason for hiding this comment

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

Seems like there is a lot of shared logic between this repo and the monorepo in how we update comments. Is it just tech debt to try to consolidate at some point or is there a reason for keeping them separate currently?

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

src/utils/wait-for-deployment-url.ts Outdated Show resolved Hide resolved
src/utils/wait-for-deployment-url.ts Show resolved Hide resolved
Co-authored-by: Daniel Book <36259973+dbook13@users.noreply.github.com>
@Que3216 Que3216 merged commit 2d18f3a into main Jul 19, 2023
5 checks passed
@Que3216 Que3216 deleted the qsh/better-errors branch July 19, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants