Add protection to syntax errors #247
Conversation
|
The only problem with this is it checks for compile errors before it pulls down the new code. Once the new code is pulled (with syntax errors), the bot is restarted and the errors cause the bot to crash. If you could do this sort of check after the startup.d/10-git-pull.sh, it would be more useful I think Just spitballing now, you could pull the code, detect it doesn't compile, send a message to the bot (before restarting it), to have the bot put a git issue up, or even try to revert the changes and identify which PR caused the issue |
|
I do not think you're right (it's the opposite of what you write). However, I have made the appropriate changes with the hope that you will now vote for them. |
startup.d/20-syntax-check.sh
Outdated
| if [ "$?" != "0" ] | ||
| then | ||
| echo "There is syntax error. This may endanger chaos. Pauses updates." | ||
| killall bash |
There was a problem hiding this comment.
I don't see how this is any better than what we're currently doing. Exiting here by killing bash will kill chaos since it replaces itself with startup.sh when it needs an update. Supervisor will make a few restart attempts, but since nothing will have changed, those will fail. What we really need is a way to test each PR before merging it.
There was a problem hiding this comment.
No, chaos will not be stopped. Chaos is not run by bash and through the supervisord. If the update process is interrupted, it will not affect the server, so server can merge a new PR which fix errors.
There was a problem hiding this comment.
Click my link. Chaos literally overwrites itself (that is, its process) with the shell script.
There was a problem hiding this comment.
Chaos overwrites itself, but running chaos use in-memory version of code. So we pull new code, check syntax, but if the code is incorrect - restart does not take place.
There was a problem hiding this comment.
- Chaos runs, merges PRs, and sees it needs an update.
- Chaos replaces itself with
startup.sh. At this point,chaos.pyis no longer running. startup.shruns, sees the new Python code doesn't compile, and kills itself.- Supervisor notices chaos died and attempts to restart it. The new code* fails repeatedly.
* I'll be honest, I haven't looked into Supervisor at all, but I would find it hard to believe that it keeps the old copy around until reread is issued. It seems much more likely to me that it will execute the newly pulled, bad code.
There was a problem hiding this comment.
@reddraggone9 , chaos.py is a daemon. Demons use in principle use the version loaded into memory. Supervisord restart the demon if them crash. If no fail there is no restart, so version loaded into memory is used. Until restart chaos.py use in-memory version.
There was a problem hiding this comment.
umm that's not a daemon. That code you linked is tied to Chaos' process. Once Chaos' process is replaced, that code is no longer running.
There was a problem hiding this comment.
"in-memory" where? Chaos' process is overwritten so that's out. It's not feasible for Supervisor to have a backup copy of the entire process and it wouldn't work even if it did. Does Supervisor somehow magically back up all the files needed to run Chaos when it first runs Chaos? Point me to the documentation which says that. If there's some other "in-memory" place that I'm missing, explain it in detail.
There was a problem hiding this comment.
Chaos' process is not replaced until the restart. Unless something is misunderstood and there is no long process.
There was a problem hiding this comment.
Chaos does spawn a child for the web server, but unless something's happening in schedule, there's only one process managing PRs. That's also the original process started by Supervisor and the one overwritten by startup.sh.
If I'm missing a place where it forks, please point me to it.
Initially when you did the syntax check, you checked the current files chaosbot is using. If the bot is running, these checks will pass. The only case where the checks fail is when we pull new code in. That occurred after your checks, so nothing different was really being done. |
startup.d/10-git-pull.sh
Outdated
| @@ -7,7 +7,7 @@ git clone "$REPO_URL" "$TEST_DIR"; | |||
| pushd "$TEST_DIR"; | |||
|
|
|||
| # Code auditing section | |||
| python -m py_compile $(find . -name '*.py'); | |||
| find . -name '*.py' -print0 | xargs -0 python -m py_compile; | |||
There was a problem hiding this comment.
python3 -m py_compile **/*.py would be cleaner.
|
It looks like this was superseded by #250. Shouldn't this be closed? |
|
@reddraggone9 Yeah. It'll be closed if the author closes it or no one comments on it in 24 hours (so please don't comment unless you have something important). |
|
I wrote that mainly for @ad-m. I know that he's online and active. |
|
🙅 PR rejected with a vote of 1 for and 4 against, with a weighted total of -3.0 and a threshold of 6.1. Open a new PR to restart voting. |
This is a protection against accidental changes that may crasj Chaous, especially the update module and self healing. It prevents, for example, the problem with Chaous as in #237. The update will be paused until the problem is fixed. However, this does not mean that the code can not be changed (the repo is still updated).