-
Notifications
You must be signed in to change notification settings - Fork 166
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
Provide pre-commit checks to ensure everything is likely to work #7
Comments
@hosungsmsft you might appreciate the script in PR #7. It's not wired into pre-commit yet, but it is faster than manually checking. |
@rgardler yes, and I was looking at your PR for review, but you already merged. I didn't understand in your script why the current branch is not embedded in the variable SCRIPT_LOCATION (contrast to BASE_TEMPLATE_URL), and you are comparing the BASE_TEMPLATE_URL with the scriptLocation value? Are they bugs? |
I need to document review processes (it’s actually in the checklist for this issue). Short version is that I believe in Commit then Review where appropriate. That is, if the code is not in the critical path and/or poses no risk then just merge it. People can review after the merge. This is a must faster process, because it’s often faster for a reviewer to make the change than it is to ask for the change.
Your observation below is a bug (I’m trying to move too fast!) If you have the time / inclination just fix it and merge your change. If you think I might be smarter than I look and have a good reason then raise it as a question as you have done here. But I can confirm they are bugs and I am not smarter than I look :-P
Ross
|
No worries, and you know I made an embarrassing bug myself (which I think led you to craft this), and fixing it urgently... Let me fix these bugs over the weekend, with broken links in other documentation. I'm right now fixing the Apache template blunder... :( |
#13 fixes the script bugs mentioned above. |
* Update install_moodle.sh * Update azuredeploy-migration.json * reverted moodle base path * .
We can provide a bunch of pre-commit checks to catch common errors and, in some cases, provide scripts to help.
The text was updated successfully, but these errors were encountered: