-
Notifications
You must be signed in to change notification settings - Fork 158
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
Refactor error handling to stop execution on errors #161
Conversation
This fixes the issue where running the unit tests invoked the entire action since index.ts calls `main()` at the top-level scope.
--ignore-path is meant to be the path of an ignore-file, not the actual directories to ignore.
The action fails to upload secrets during one of the testing steps since the Worker doesn't exist yet.
The action fails to upload secrets during one of the testing steps since the Worker doesn't exist yet.
164d639
to
d7637bf
Compare
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 seems like a premature abstraction, the file is far from being large and colocating the functions means not having to jump around files.
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.
Importing anything from index.ts
has the side-effect of executing the action since main()
is executed in top-level scope. This meant that running the vitest test suite to test these util functions was inadvertently executing the entire action. Pulling these into their own file felt like the least disruptive way around that, especially since they’re already being unit tested in isolation. Alternatively I could have renamed index.ts
to main.ts
and added a new index.ts
that just imports and executes main()
, but that would have made this PR harder to review and I didn’t want to make the diff any noisier than it’s already become.
@@ -9,12 +9,12 @@ | |||
"isolatedModules": true, | |||
"target": "ESNext", | |||
"module": "ESNext", | |||
"moduleResolution": "NodeNext", | |||
"moduleResolution": "Bundler", |
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.
Oh that is cool! I didnt see they had a new moduleResolution bundler
Fixes #160
This PR refactors the way errors are propagated through the action when encountered. Previously, the action relied solely on
core.setFailed()
to handle errors, but this only sets the action status while allowing execution to continue. You can see this occur in https://github.com/cloudflare/wrangler-action/actions/runs/5968662403/job/16192935017#step:9:22, where the secret upload step failed, but the action proceeded to deploy the worker anyway.Now instead of calling
core.setFailed()
at the source of every failure case, we throw errors and catch them at the top level where we then output the error and callcore.setFailed()
.This PR also:
finally
blocks around someendGroup()
calls to ensure no groups are left dangling open if an error occurs.main()
call at the top-level scope ofindex.ts
being invoked when running the unit tests.moduleResolution: "Bundler"
)--ignore-path