bakerkretzmar / laravel-deploy-preview

A GitHub Action to deploy PR preview sites for Laravel apps.
MIT License
19 stars 6 forks source link

Request failed with status code 422 when PR branch name includes slashes #21

Closed GlitchWitch closed 10 months ago

GlitchWitch commented 10 months ago

When a PR branch includes slashes the action fails with the following output:

Creating new deploy preview site named 'example/1234_12-test'

file:///home/runner/work/_actions/bakerkretzmar/laravel-deploy-preview/main/webpack:/laravel-deploy-preview/node_modules/axios/lib/core/settle.js:19
    reject(new AxiosError(
^
AxiosError: Request failed with status code 422
    at settle (file:///home/runner/work/_actions/bakerkretzmar/laravel-deploy-preview/main/webpack:/laravel-deploy-preview/node_modules/axios/lib/core/settle.js:19:1)
    at IncomingMessage.handleStreamEnd (file:///home/runner/work/_actions/bakerkretzmar/laravel-deploy-preview/main/webpack:/laravel-deploy-preview/node_modules/axios/lib/adapters/http.js:572:1)
    at IncomingMessage.emit (node:events:526:35)
    at endReadableNT (node:internal/streams/readable:1376:12)
    at processTicksAndRejections (node:internal/process/task_queues:82:21)

To resolve this, the action should account for special characters such as / in branch names and normalise them before utilising them.

GlitchWitch commented 10 months ago

Hmm, I'm starting to run into this with other branches as well so it may not be strictly branches with odd special characters.

For example: snyk-upgrade-7e6702738dc4e4ea185d5473191ae7e1 succeeded, but snyk-upgrade-93032a7d1412d8fcfe8f26791b9eeff7 fails with the same 422 error.

Creating new deploy preview site named 'snyk-upgrade-93032a7d1412d8fcfe8f26791b9eeff7'

file:///home/runner/work/_actions/bakerkretzmar/laravel-deploy-preview/main/webpack:/laravel-deploy-preview/node_modules/axios/lib/core/settle.js:19
    reject(new AxiosError(
^
AxiosError: Request failed with status code 422
    at settle (file:///home/runner/work/_actions/bakerkretzmar/laravel-deploy-preview/main/webpack:/laravel-deploy-preview/node_modules/axios/lib/core/settle.js:19:1)
    at IncomingMessage.handleStreamEnd (file:///home/runner/work/_actions/GlitchSecure/laravel-deploy-preview/main/webpack:/laravel-deploy-preview/node_modules/axios/lib/adapters/http.js:572:1)
    at IncomingMessage.emit (node:events:526:[35](https://github.com/REDACTEDORG/REDACTEDREPO/actions/runs/6605367646/job/17940464752#step:3:36))
    at endReadableNT (node:internal/streams/readable:1[37](https://github.com/REDACTEDORG/REDACTEDREPO/actions/runs/6605367646/job/17940464752#step:3:38)6:12)
    at processTicksAndRejections (node:internal/process/task_queues:82:21)

Sadly even with debug logging turned on there isn't enough information here to figure out why this is so unreliable.

I checked forge and do not see the site, as that was previously one of the causes (the site already existing, but runs failing with a 429)


Edit It seems this was caused by the database existing but not the site existing from a previous run. It may be good to add a check here to destroy any existing database and site with a conflicting name before creating a new one.

bakerkretzmar commented 10 months ago

Some of this is fixed in v2.1.3. The 429s were Forge rate-limiting the action, which I've now added automatic retries to handle.

Do you know how you ended up with an existing database that hadn't been deleted? It should have been deleted when the PR that created it was closed, do you know if that had previously failed?

I'm also handling validation errors a bit better now, so you should see a reason in the logs. I'm hesitant to delete conflicting databases just in case they weren't created by this action...

GlitchWitch commented 10 months ago

It seemed like the database was created during an action run when several other actions were also running, something 429'd, then no amount of opening/closing could overcome the conflict of the database existing and not the site.

More debugging and retries will probably help.

bakerkretzmar commented 10 months ago

Ah yeah that makes sense, another run creating the database and then failing on something else. Actually I guess we could check if the database already exists and just skip creating it, as opposed to deleting it immediately. I'll look into that.

bakerkretzmar commented 10 months ago

I think now that there's better error handling and debug output about what causes this error I'm okay leaving it as is. I don't like the idea of deleting or re-using existing databases, if there's already a database with the name we're trying to create that feels to me like something a human should go take a look at just in case. Happy to revisit this if it continues to be an issue though.