Allow configuration to fail when a hook execution fails, fixes #2536#2652
Merged
Conversation
Member
|
Look at you go! |
Member
|
Please improve this PR description with full manual testing instructions, including example hooks, and please confirm that you've tested it manually. I'm sure this will need a test as well. We can cross that bridge a little farther down the road. |
Contributor
Author
Done 👍 |
I discovered a panic in testing, so added this code
c9f70ce to
6c4c124
Compare
rfay
approved these changes
Dec 28, 2020
rfay
left a comment
Member
There was a problem hiding this comment.
Nice work on this!
I did these things you may want to review:
- Added test coverage for this case
- Prohibit exec and composer tasks in pre-start. This was a fail I discovered in testing this and had nothing to do with the PR.
- Fixed a panic where non-string values were used as the value of an exec, like
exec: false. This again was a panic I discovered in testing and had nothing to do with this PR.
Would love it if you could give a look and a test and make sure you're good with it.
Thanks for the nice work on this!
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Problem/Issue/Bug:
#2536
How this PR Solves The Problem:
Introduction of a new option (global & local): fail-on-hook-fail
Manual Testing Instructions:
Easiest way to test is probably by using:
ddev config global --fail-on-hook-fail=trueAfter that, just include a pre-start hook with an exit code (!=0) for instance use the following script saved as test.sh in the config.yaml of your project like this for example:
The contents of the example
test.shcan be found here (place in root folder of your project)After you have done all that make sure, you are using the self-build version of
ddeveither from my branch or from the PR, runddev poweroffto ensure, that there are no side-effects from other versions and start your project as usual by callingddev startAutomated Testing Overview:
make staticrequiredfinished without errors and without warnings.Manual testing was successful.
Tested:
ddev config global --fail-on-hook-fail=truefail_on_hook_fail: trueto projects.ddev/config.yamlRelease/Deployment notes:
Nothing else should be affected, even though this was my first project using "Go" 🙂