Update docs and hooks worker client for go hooks #505
Conversation
ddelnano
force-pushed
the
add-go-hook-handler
branch
from
fbc6f9c
to
bcd8240
Compare
|
@honzajavorek The PR this depended on is finally merged |
|
@ddelnano Great! I asked our Go lover @pksunkara to look at it once he has a free moment, so expect him to land & comment here soon. |
|
@honzajavorek deleted my previous comments, cause I thought this was on an issue and not this PR. Sounds good to me 👍 |
| ``` | ||
|
|
||
| ## Usage | ||
|
|
||
| Using Dredd with Go is slightly different to other languages, as a binary needs to be compiled for execution. Instead of providing a variety of hookfiles to run, your own binary is built using the Go hooks library `goodman` to run your specific hooks. The path to this binary is passed to dredd via the `--language` flag. You may also need to supply a `--hookfiles` flag to get the tests to run, but the value of this is of no matter (the dredd runner requires it). | ||
| Using Dredd with Go is slightly different to other languages, as a binary needs to be compiled for execution. The --hookfiles flags will be compiled hook binaries. See below for an example hooks.go file to get an idea of what this would look like. |
There was a problem hiding this comment.
- hookfiles flags will be compiled hook binaries
+ hookfiles flags should point to compiled hook binaries.- get an idea of what this would look like
+ get an idea of what the source file behind the go binary would look like.There was a problem hiding this comment.
@pksunkara can you be more specific? Is this just not clear?
There was a problem hiding this comment.
I just made the sentences a bit more clear. My comment has the updated sentences which you need to copy.
There was a problem hiding this comment.
@pksunkara awesome thanks for clarifying. I updated it.
|
Just 2 minor comments regarding docs. Will merge it after they are fixed. |
|
@pksunkara @honzajavorek I updated the PR addressing the feedback before this get merged, however, I am thinking there might be something that might need addressed first. Dredd crashes with the Go hooks if the API Blueprint transaction has an empty request body. See the difference between the Go tests and the template tests |
|
Here is an example of the dredd crash, I am going to try and prepare a fix.
|
|
@honzajavorek, @pksunkara not sure exactly what the deal is with the findings in my previous comments. I wrote out a full example using a sample go app, goodman, and dredd compiled from this branch and blueprint files that have empty request bodies run fine. You can see the example here. Either of you could run the example if you wanted to following the instructions in the Wiki to verify my claims that it works as expected, however, you would need to update the path in the Makefile since I needed to use dredd compiled from this branch (since it uses these changes). Let me know what you think. |
|
Does the above crash happen with other language hook files or only golang? Depending on that, we need to fix it in a separate PR. I am merging this for now. |
|
@pksunkara when I developed the php hooks I never ran into this. I also tried to reproduce it with a real example instead of the cucumber tests (where I was seeing it) and I was unable to do so. I opened a PR for another crash that I found that indicates a hook server failure ( #511 ). This would just provide better information to the hook server dev instead of seeing the error |
|
@pksunkara @honzajavorek when do the pre releases go out? I would like to merge my example in the goodman repo but since npm's support for installing repos via commit hashes is lacking (coffeescript would not be compiled) I can't merge until this change is in a new release. |
|
@ddelnano I should release another one within a day. |
|
awesome thanks! |
This PR depends on this. Once that is merged, the go hooks should be considered compliant with the hooks for other languages: php, ruby, python, perl, etc.
I tested the changes with this repo. I could not install dredd from a github repo since npm does not run prepublish and the necessary commands to do so I made these commits on my fork and made sure the examples worked.