search

JamesIves / github-pages-deploy-action

🚀 Automatically deploy your project to GitHub Pages using GitHub Actions. This action can be configured to push your production-ready code into any branch you'd like.
https://github.com/marketplace/actions/deploy-to-github-pages
MIT License
4.22k stars 357 forks source link

Check for fatal on every line of stderr of the git command #1551

Closed ben-z closed 4 months ago

ben-z commented 5 months ago

Description

Currently, we ignore git push errors and only fail when we find specific substrings in stdout or stderr starts with fatal:. This is insufficient to catch all errors because sometimes fatal: can occur on lines that are not the first. For example:

/usr/bin/git push --porcelain ***github.com/WATonomous/infra-config.git github-pages-deploy-action/mdm3v520a:data
remote: Write access to repository not granted.
fatal: unable to access 'https://github.com/WATonomous/infra-config.git/': The requested URL returned error: 403
Changes committed to the data branch… 📦
Running post deployment cleanup jobs… 🗑️
/usr/bin/git checkout -B github-pages-deploy-action/mdm3v520a
Reset branch 'github-pages-deploy-action/mdm3v520a'
/usr/bin/chmod -R +rw github-pages-deploy-action-temp-deployment-folder
/usr/bin/git worktree remove github-pages-deploy-action-temp-deployment-folder --force
Completed deployment successfully! ✅

In the output above, the fatal: message occurs on the second line.

Testing Instructions

Run this action without giving the job contents: write permission. Before this change, the job will silently fail. After this change, the job will raise an exception as expected.

Additional Notes

The approach of ignoring all issues except some known ones is very error prone. We should instead do the opposite: ignore all known non-issues and raise an exception for everything else. But this takes more work and may be a breaking change for some use cases until we can gather a complete list of non-errors.

JamesIves commented 5 months ago

Thanks for the PR! I like this change and I agree with the philosophy, I see this was also bought up in #1472 also. It's sometimes hard to really predict what an actual error is here as there doesn't seem to be a standardized way of checking for them outside of just doing a string match.

I'll run this through the integration suite and bundle this with a few long-standing dependency changes I've been meaning to make, I'll likely wrap this up once I am back from my upcoming trip.